Add webpack 5 support to dependency-extraction-webpack-plugin #27533
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: 0 B Total Size: 1.21 MB ℹ️ View Unchanged
|
jsnajdr
force-pushed
the
webpack5/dependency-extraction-plugin
branch
from
60aa47d to
b7ef25a
Compare
|
@gziolo @sirreal This is still a draft. I'll make the PR official after the webpack 5 upgrade PR (#26382) that's stacked on top of this one starts working correctly (there are still some unit test failures) and actually proves that the webpack 5 support works 🙂 I also plan to update the PR description so that reviewers know what I changed and why. |
|
Ready for review now 🎉 |
|
Great work, it's very complex set of changes. It might take me some time to review but it's now on my list 👍 |
I did my best to split the patch into small independent commits and explain in detail what they do and why. Let me know if there's something that still looks confusing. |
gziolo
added
the
[Tool] Dependency Extraction Webpack Plugin
…them explicitly Making the plugin typecheck correctly is an impossible task, due to incomplete webpack types and the need to be compatible with two versions at once. Let's write the types explicitly in a `types.d.ts` file and stop creating them at build time.
jsnajdr
force-pushed
the
webpack5/dependency-extraction-plugin
branch
from
b7ef25a to
0fe7332
Compare
gziolo
approved these changes
Dec 8, 2020
There was a problem hiding this comment.
I'm not an expert in how TypeScript integration should look like but your explanation makes a lot of sense. I would defer to @sirreal who typed this package before. Code wise, everything seems to work as necessary - all test pass. Well, at least for webpack 4 that it still uses. However, I believe that it works with webpack 5 as well since it was extracted from the PR where the upgrade is handled.
| // Ignore reason: json2php is untyped | ||
| // @ts-ignore | ||
| const webpack = require( 'webpack' ); | ||
| const { RawSource } = webpack.sources || require( 'webpack-sources' ); |
There was a problem hiding this comment.
How about we document it in the code:
Suggested change
| const { RawSource } = webpack.sources || require( 'webpack-sources' ); | |
| // With webpack 5 it uses `sources` field but for webpack 4 it has to fallback to `webpack-sources` package. | |
| const { RawSource } = webpack.sources || require( 'webpack-sources' ); |
or something like that.
… a package webpack 5 exposes `webpack-sources` as `webpack.sources` property, and makes sure that it's exactly the version that you need.
…e concatenated modules
…pe field In webpack 5, `ExternalModule` instanced no longer have the `external` boolean flag. We need to identify them by the `externalType` field presence.
In webpack 5, the default options are not applied yet when `Plugin.apply` is called. Postpone reading the `output.filename` option to the `emit` hook where the value is used. See also: https://webpack.js.org/blog/2020-10-10-webpack-5-release/#new-plugin-order
jsnajdr
force-pushed
the
webpack5/dependency-extraction-plugin
branch
from
0fe7332 to
9453ab2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #26134. Adds support for webpack 5 (while keeping webpack 4 compat) to
dependency-extraction-webpack-plugin. Spinoff from #26382. Description of changes I had to make follows.Remove type annotations, commit a hand-crafted
types.d.tsAfter discussion with @sirreal in #26382 (comment), I decided to stop typechecking the plugin source code with TypeScript. Webpack types are not complete enough to make it work, and making a plugin that supports both webpack 4 and 5 is an impossible task. To make types available for consumers of the plugin, I added a hand-crafted
types.d.tsfile that describes the plugin's public interface, i.e., the constructor and its options. And it works in my Atom IDE:Stop using the
webpack-sourcespackage if possibleDepending on and importing the
webpack-sourcespackage is legacy now. A plugin can access the APIs on awebpack.sourcesproperty of thewebpackpackage export.Descend into concatenated modules
The
ExternalModuleinstances can now be concatenated into a common module. We need to descend intoConcatenatedModule.modulesarray to find all externals.Don't read compiler options before defaults were applied
In webpack 5, the default options are not applied yet when
Plugin.applyis called. Postpone reading theoutput.filenameoption to theemithook where the value is used. See also: https://webpack.js.org/blog/2020-10-10-webpack-5-release/#new-plugin-orderThen there are changes that are in unit tests only:
Change how
ExternalModuleinstances are identifiedIn webpack 5,
ExternalModuleinstanced no longer have theexternalboolean flag. We need to identify them by theexternalTypefield presence.Sort modules by
.identifier()rather than implicitly by.toString()Produces more stable sorting of the modules, preventing test failures due to implementation details changes. In webpack 5, the
toString()output includes adebugIdvalue, which is an incremental counter incremented on eachModuleinstantiation. Looking at.identifier()produces more stable sorting key.Remove the
with-externsunit test that depends on webpack internalsThere was a
with-internsunit test that checked for behavior of twoExternalsPlugininstances, one from config, one from this plugin, that match the same request. But this tests webpack internals rather than anything else.In webpack 4, the module factories were processed by a
SyncWaterfallHook, using the last factory value produced by the hook for a given request. In webpack 5, the hook is anAsyncSeriesBailHook, and it bails on the first plugin returning a valid factory.So, even if two conflicting
ExternalsPlugininstances were declared in the same order, the behavior is different in v4 vs v5.How to test:
The full webpack 5 upgrade is stacked on top of this PR in #26382, and it builds cleanly including all unit and e2e tests.
This PR alone still uses webpack 4 with the updated
dependency-extraction-webpack-plugin. And is also green.