[WIP] webpack@5 `hooks` support #166

liangchunn · 2018-10-14T12:21:54Z

In webpack@5, custom hooks are no longer allowed to be added directly onto this.compiler.hooks. WeakMap + static getHooks() should be used instead (More details here: https://github.com/webpack/webpack/projects/5#card-10390019)

This is a WIP, please give feedback on what should be updated.

Changed

(webpack5): Use static getHooks()
(webpack5): Use require('webpack').version when available in test/integration/webpackVersion.js
(ci): Add [email protected] into CI env
(docs): Add migration section for webpack4+ hooks
(package): remove support for node6
(webpack5): rename hooks

Resolved

(package): Decide if webpack5 support should be added into existing version/have breaking major version
~~(webpack5): Decide if _pluginCompat should be removed (*)~~ Removed _pluginCompat
~~(ci): Decide if we should reorder yarn add $WEBPACK $TSLOADER $VUELOADER -D in travis.yml~~
~~(ci): Skip tests for node6 + webpack@5~~

Resolved in #169 instead

~~- [x] (ci): Lint in CI~~
~~- [x] (lint): Upgrade TSLint and ESLint~~
~~- [x] (lint): Lint *.ts and *.js files~~
~~- [x] (tests): don't use object spread (node6 does not support this: https://node.green/#ES2018-features-object-rest-spread-properties)~~

~~(*) I don't really know what _pluginCompat does, removing it doesn't cause build failures on CI~~

src/hooks.ts

src/index.ts

src/tsconfig.json

test/integration/vue.spec.js

johnnyreilly · 2018-10-14T14:35:59Z

First of all, thanks for your work here; this is awesome! I've put some adhoc comments on the PR but I've realised a few things.

First of all there's quite a lot of "noise" in this PR due to formatting changes / linting related stuff. I'd really like to separate out the webpack 5 support changes from that.

We use prettier in ts-loader; (see the package.json for details: https://github.com/TypeStrong/ts-loader/blob/master/package.json - including a pre-commit hook).

I've long thought I'd like to apply prettier in fork-ts-checker-webpack-plugin. I'd be happy to do that, get it merged in and then have you merge that into your PR and work against that.

Or you could apply prettier yourself in a separate PR?

Or you could just drop all the styling changes entirely from this PR. I honestly don't mind which; but I'd like this PR to be webpack 5 focussed only if that's okay!

Sorry - this may sound ungrateful; but I'm just trying to make it easier to focus on doing a good review and catching the right issues. At present I'm quite distracted by all the formatting changes!

johnnyreilly · 2018-10-14T14:36:50Z

Can you confirm if this supports webpack 4 as well as 5 please? I think it might; but I'm not certain.

liangchunn · 2018-10-14T14:47:52Z

@johnnyreilly
I've added tests for webpack#next in TravisCI. It's working with both webpack 4 and 5. I'll drop the styling changes and add in prettier and probably with lint-staged and husky in another PR.

Are there any comments on the _pluginCompat? I still can't find any documentation on it though.

johnnyreilly · 2018-10-14T15:07:15Z

Are there any comments on the _pluginCompat? I still can't find any documentation on it though.

Do you mean the support for webpack 2/3?

liangchunn · 2018-10-14T15:15:32Z

Yes and no.

If you look into this line on master (https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/master/src/index.ts#L189-L191), the _pluginCompat will never be reached if it's webpack < 4

johnnyreilly · 2018-10-14T16:02:26Z

If you look into this line on master (https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/master/src/index.ts#L189-L191), the _pluginCompat will never be reached if it's webpack < 4

I see. Take off and nuke _pluginCompat from orbit; it's the only way to be sure. 😄 (By which I mean: feel free to remove _pluginCompat - it's clearly unused)

johnnyreilly · 2018-10-14T16:38:26Z

~~Can you provide some more information about static getHooks() please? I tried your link but it didn't have anything that seems related.~~

I'm curious as to how the webpack 4 back compatibility works.

liangchunn · 2018-10-14T17:22:29Z

@johnnyreilly

I've been trying to understand why this change was made in webpack@5 as well... Unfortunately webpack@5 is a major change, most plugins will definitely break, and you might want to publish a major version of this plugin with no backwards compatibility.

This is because webpack@5 is removing hooks on compiler.hooks. Any plugin that depends on a particular plugin, which supports webpack@5, has to be updated as well!

This means that plugins like fork-ts-checker-notifier-webpack-plugin will no longer be able to tap into the events that fork-ts-checker-webpack-plugin exposes.

Example (https://github.com/johnnyreilly/fork-ts-checker-notifier-webpack-plugin/blob/master/index.ts#L101):
Tapping into compiler.hooks.forkTsCheckerReceive will yield nothing, since hooks are not installed into the webpack compiler instance, but rather on the static property on the class, ie: ForkTsCheckerWebpackPlugin.getHooks()

I am going to close this PR.
Since webpack@5 is still not done, we do not have any certainty on how the new plugin system works. At least we have some knowledge about it though.

johnnyreilly · 2018-10-14T17:38:42Z

Hey @liangchunn ,

Actually - I'd quite like to leave this PR open if that's okay with you? We're not ready to ship but I suspect we're very close! Also, doesn't the:

    let hooks = compilerHookMap.get(compiler);
    if (hooks === undefined) {

provide the webpack 4 compatibility? That seems to be implied in this PR: webpack/webpack#7672

Actually - I'm not totally clear; but I feel this could well become the webpack 5 support we need in future!

liangchunn · 2018-10-14T17:44:21Z

I don't think that those lines provide webpack 4 compat. That PR changes its own HMR plugins which is shipped with webpack@5, AFAIK.

johnnyreilly · 2018-10-14T18:03:24Z

Ah right; either way, the fact we got passing tests with all versions of webpack was very encouraging!

liangchunn · 2018-10-14T18:09:45Z

One way we could provide webpack@5 support is to use Object.isFrozen() on compiler.hooks, and subsequently injecting hooks into compiler.hooks when it's not frozen, and using getHooks when it is frozen. I fear that this might complicated the logic behind the exposed plugins.

I'll experiment again when I have some free time on hand.

liangchunn · 2018-10-15T09:33:06Z

I've added tests against node6, node8, and node10 in my own branch and made tests againts webpack#next non-failing. It seeems like webpack@5 only supports node>=8.9.0.

error [email protected]: The engine "node" is incompatible with this module. Expected version ">=8.9.0".
error Found incompatible module

johnnyreilly · 2018-10-15T13:57:10Z

I've added tests against node6, node8, and node10 in my own branch and made tests againts webpack#next non-failing.

Awesome!

It seeems like webpack@5 only supports node>=8.9.0.

Yup sounds about right.

johnnyreilly · 2018-12-21T15:43:54Z

Hey @liangchunn,

Would you be up for continuing your work now 5 alpha is out?

webpack/webpack#8537

sokra · 2018-12-21T15:46:52Z

src/hooks.ts

+
+function createForkTsCheckerWebpackPluginHooks(): Record<keyof typeof customHooks, SyncHook | AsyncSeriesHook> {
+    return {
+        forkTsCheckerServiceBeforeStart: new AsyncSeriesHook([]),


You could use shorter names now, as it's already scoped to this plugin:

Suggested change

forkTsCheckerServiceBeforeStart: new AsyncSeriesHook([]),

serviceBeforeStart: new AsyncSeriesHook([]),

sokra · 2018-12-21T15:47:52Z

src/index.ts

@@ -156,6 +142,10 @@ class ForkTsCheckerWebpackPlugin {
    this.vue = options.vue === true; // default false
  }

+  static getHooks(compiler: webpack.Compiler) {


Suggested change

static getHooks(compiler: webpack.Compiler) {

static getCompilerHooks(compiler: webpack.Compiler) {

That would be our naming convention, but technically you are free to choose whatever you like.

We have chosen this naming to allow adding hooks for other objects later, i. e. getCompilationHooks.

liangchunn · 2018-12-21T16:49:43Z

@johnnyreilly Will do when I have the time!

johnnyreilly · 2018-12-21T17:42:16Z

Awesome!

johnnyreilly · 2018-12-22T09:07:17Z

Hey @liangchunn ,

I just tried to make your life easier by catching the branch up with upstream changes. For some reason the GitHub UI is reporting conflicting files.... I'm not sure why; it's fine when I build locally.

Maybe it's just easier to reapply your changes (with @sokra's feedback) on a new PR. Not sure... Apologies if I've just made things harder... 😢

liangchunn · 2018-12-22T13:06:01Z

@johnnyreilly

I've re-applied all my changes with the current master branch.
This commit will be a breaking change for all other plugins that consume this plugin along with webpack 4+

Hook naming

Now for the naming, as @sokra suggested, we could rename the all the hooks since it's now scoped by default:

src/hooks.ts
-  forkTsCheckerServiceBeforeStart: 'fork-ts-checker-service-before-start',
+  serviceBeforeStart: 'fork-ts-checker-service-before-start',
-  forkTsCheckerCancel: 'fork-ts-checker-cancel',
+  cancel: 'fork-ts-checker-cancel',
-  forkTsCheckerDone: 'fork-ts-checker-done',
+  done: 'fork-ts-checker-done',

Which means that tapping into the plugin's events will change (for webpack 4+):

-  compiler.hooks.forkTsCheckerDone.tap(...args)
+  const forkTsCheckerHooks = ForkTsCheckerWebpackPlugin.getCompilerHooks(compiler)
+  forkTsCheckerHooks.done.tap(...args)

I've introduced a migration section in README.md, but I have yet to make any changes regarding the hook's naming. If you green-light the proposed changes, I'll commit accordingly.

Tests Failures on node 6

There are some issues with the build with node 6 only:

In travis.yml, yarn add $WEBPACK $TSLOADER $VUELOADER -D is called after yarn install which causes yarn to install [email protected] from yarn.lock first, and fails all builds
- Possible solution: reorder commands in travis.yml
webpack5 builds should skipped
- Possible solution: use matrix.exclude in travis.yml as described here

johnnyreilly · 2018-12-22T16:38:19Z

First of all, amazing work @liangchunn!

To your points:

This commit will be a breaking change for all other plugins that consume this plugin along with webpack 4+

I think that's fine. This is a major version bump for webpack. It's reasonable that we have the same here.

Hook naming

I think renaming the hooks as suggested by @sokra is a good idea. Particularly if we're going to go for a breaking changes version bump.

Let's do it. I maintain https://github.com/johnnyreilly/fork-ts-checker-notifier-webpack-plugin which depends upon this. I'm happy to make the required changes around hook renaming there.

Tests Failures on node 6

I think it's time to drop support for node 6. webpack 5 drops support for it and the minimum recommended download of nodejs is node 10. Further to that, node 6 is being end of lifed in 3 months time.

Can you remove node 6 from the Travis test matrix please?

Also could you align the engines section of the package.json with the equivalent webpack 5 package.json please? i.e. update this:

  "engines": {
    "node": ">=6.11.5"
  },

Regarding what version number we should bump the plugin to, I've long felt that it's time this plugin should have a major version number; 1.0.0 rather than 0.6.x

What do you think? cc @piotr-oles

…ebpack5

liangchunn · 2018-12-23T13:44:42Z

Regarding what version number we should bump the plugin to, I've long felt that it's time this plugin should have a major version number; 1.0.0 rather than 0.6.x

I'd fully agree to this. Recently upgraded from 0.4.x to 0.5.x and it broke the build system in typescript-node-scripts (due to automatic dep upgrades from Greenkeeper), and it took me a few days to find the cause of it.

…pings

liangchunn · 2018-12-23T14:21:43Z

@johnnyreilly
Ready for review

In 8773e6b and af98c77, I've:

removed node6 from travis.yml
updated engines.node to >=8.9.0 (same with webpack5)
renamed all the hooks from forkTsCheckerXXX to XXX
moved webpack2/3 string hooks into hooks.ts as legacyHookMap
improved typings on WeakMap and exported hooks

johnnyreilly · 2018-12-23T16:24:40Z

Tremendous work @liangchunn! I've hacked on your branch and:

set the version number to 1.0.0-alpha.0
added a tag: next to the .travis.yml so we can publish this package with a next dist tag.
updated the changelog to add details of the new version alongside the breaking changes / migration guide info which I moved from the README.md

I think we're pretty good to push this out as an alpha release. Is there anything else you think we should get in there first?

Also, I've been thinking about how to upgrade fork-ts-checker-notifier-webpack-plugin and I'm not sure how to upgrade it. Perhaps you can advise? The getCompilerHooks method is marked as private so I'm guessing I'm not supposed to be able to access it... Is that right?

   private static getCompilerHooks(compiler: webpack.Compiler) {
     return getForkTsCheckerWebpackPluginHooks(compiler);
   }

liangchunn · 2018-12-23T16:54:42Z

@johnnyreilly

The getCompilerHooks method is marked as private so I'm guessing I'm not supposed to be able to access it... Is that right?

It should be exposed, I messed up 😅. Moved it to a public method in c410927.
Didn't notice this since test files are written in JS and there's no type checking for private method access.

Also, I've been thinking about how to upgrade fork-ts-checker-notifier-webpack-plugin and I'm not sure how to upgrade it. Perhaps you can advise?

You would want to do:

import * as forkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin'

forkTsCheckerWebpackPlugin.getCompilerHooks(compiler).tap('...', () => {})

EDIT: I've tried it locally and the tests seem to pass

AFAIK there isn't anything left to do, I've tested this against a real project (without tapping into hooks) and everything is working fine for me!

johnnyreilly · 2018-12-23T17:15:27Z

That's awesome - let's get this out there! Thanks for all your hard work 🌻♥️

johnnyreilly · 2018-12-23T17:31:49Z

Should be available on npm now - can you confirm? It's published as @next

liangchunn · 2018-12-23T17:48:48Z

Yep! It's there: https://www.npmjs.com/package/fork-ts-checker-webpack-plugin?activeTab=versions

.travis.yml

johnnyreilly · 2019-01-20T08:41:54Z

@alfaproject you might be interested to learn that fork-ts-checker-webpack-plugin is node 6 compatible again as of https://github.com/Realytics/fork-ts-checker-webpack-plugin/releases/tag/v1.0.0-alpha.4

We can't add it back to the travis test matrix as the project will not build on node 6 due to webpack 5 dependency issues. It runs just fine though as long as you don't use the new measureCompilationTime feature which depends upon node 8+. The engines section has been updated back to:

  "engines": {
    "node": ">=6.11.5"
  },