Use same hook for both webpack 4 and 5

[Levdbas]

This PR contains:

• [x] bugfix
• [ ] feature
• [ ] refactor
• [ ] documentation
• [ ] other

Are tests included?

• [ ] yes (bugfixes and features will not be merged without tests)
• [x] no (using same tests as provided)

Breaking Changes?

• [ ] yes (breaking changes will not be merged unless absolutely necessary)
• [x] no

List any relevant issue numbers: #245

Description

emitHook is now running on compiler.hooks.emit.tap(hookOptions, emit); for webpack 4/5. This fixes #245 As said in the issue, I have little experience with webpack plugins, but the tests seems to work for both webpack 4 and 5.

[Levdbas]

The CI tests are not running the webpack v5 testsuite but when running locally all 49 tests pass.

[Levdbas]

PR now contains the new useLegacyEmit option. I also added a small readme text. It might be smart to add a test but I've tested it locally and it just works. Since my knowledge about ava is non-existent I hope you can take a look at that.

Circleci fails on the security audit btw.

[shellscape]

@Levdbas please merge from upstream/master for the audit fix. We're going to need a test specifically for using webpack v5 with the plugin and useLegacyEmit: true before I can merge this. Using the existing tests aren't sufficient to cover that scenario.

[Levdbas]

Upstream is merged. Any idea how to make this test work? Include a plugin from which we know only emits on the legacy hook and lock the version? I have no experience with AVA yet so I will see what I can do.

[shellscape]

Ava is very similar to Jest, the learning curve is small. That sounds like a good test plan to me.

[Levdbas]

Well that was fairly straightforward. Let me know if everything is in order this way.

[Levdbas]

Hi @shellscape , did you had the chance to take a look at the test?

[Levdbas]

Hi @shellscape , any word if and when you can merge this and release this in a new version?

[shellscape]

Yeah this PR is going to be part of the last release of support for webpack v4 and Node v10. Stand by.

[privatenumber]

I'd like to chime in and say that I'm troubled by the hook discrepancy as well.

I wrote a localization plugin, which duplicates assets and localizes them (essentially just string replacement) after the optimization/minification stage (because we don't want minification to repeat for the duplicated assets).

For WP4, the localization runs in the afterOptimizeChunkAssets sync hook, which works fine with webpack-manifest-plugin because it runs on the compiler.emit hook which comes afterwards.

For WP5, the localization runs in the afterProcessAssets hook, which doesn't work with webpack-manifest-plugin because webpack-manifest-plugin runs at the end of the processAssets hook.

The reason why I need to hook into afterProcessAssets instead of processAssets in WP5 is because minifiers can enable the additionalAssets option to re-run on new assets emitted by other plugins in processAssets. (eg. Terser, esbuild-loader).

That said, I might recommend not using the processAssets hook for this as more assets can technically be emitted in afterProcessAssets. I think compilation.afterProcessAssets makes more sense for this plugin. If not, definitely enable additionalAssets in processAssets so new assets can be caught.

Edit: After more investigation, maybe I can use processAssets as Terser skips re-minification. Still, I think afterProcessAssets is the right hook for this plugin.

---

so we put this different hook in for v5 based on direction from the webpack team itself.

@shellscape Curious what reasoning they gave you?

[shellscape]

@privatenumber I honestly can't remember now what the feedback was, it's just been too long. One of those "I remember why, but not what" situations. That said, I'm all for using afterProcessAssets. Sounds completely reasonable to me. If you'd like to open a PR to make that change, I'd be happy to accept it (pending passing CI, etc of course). I'm going to merge this one for the time being, which should help some folks out.