Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add webpack 5 support to dependency-extraction-webpack-plugin #27533

Merged
merged 10 commits into from Dec 8, 2020

Conversation

@jsnajdr
Copy link
Contributor

@jsnajdr jsnajdr commented Dec 6, 2020

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.ts
After 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.ts file that describes the plugin's public interface, i.e., the constructor and its options. And it works in my Atom IDE:

Screenshot 2020-12-04 at 21 53 22

Stop using the webpack-sources package if possible
Depending on and importing the webpack-sources package is legacy now. A plugin can access the APIs on a webpack.sources property of the webpack package export.

Descend into concatenated modules
The ExternalModule instances can now be concatenated into a common module. We need to descend into ConcatenatedModule.modules array 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.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

Then there are changes that are in unit tests only:

Change how ExternalModule instances are identified
In webpack 5, ExternalModule instanced no longer have the external boolean flag. We need to identify them by the externalType field 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 a debugId value, which is an incremental counter incremented on each Module instantiation. Looking at .identifier() produces more stable sorting key.

Remove the with-externs unit test that depends on webpack internals
There was a with-interns unit test that checked for behavior of two ExternalsPlugin instances, 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 an AsyncSeriesBailHook, and it bails on the first plugin returning a valid factory.

So, even if two conflicting ExternalsPlugin instances 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.

@github-actions
Copy link

@github-actions github-actions bot commented Dec 6, 2020

Size Change: 0 B

Total Size: 1.21 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 128 kB 0 B
build/block-editor/style-rtl.css 11.2 kB 0 B
build/block-editor/style.css 11.2 kB 0 B
build/block-library/editor-rtl.css 9.07 kB 0 B
build/block-library/editor.css 9.07 kB 0 B
build/block-library/index.js 149 kB 0 B
build/block-library/style-rtl.css 8.35 kB 0 B
build/block-library/style.css 8.35 kB 0 B
build/block-library/theme-rtl.css 789 B 0 B
build/block-library/theme.css 790 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 10.1 kB 0 B
build/core-data/index.js 15.4 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.97 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 769 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.95 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.49 kB 0 B
build/edit-post/style.css 6.47 kB 0 B
build/edit-site/index.js 24.7 kB 0 B
build/edit-site/style-rtl.css 3.93 kB 0 B
build/edit-site/style.css 3.93 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 43.4 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.74 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.93 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.31 kB 0 B
build/notices/index.js 1.82 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 2.92 kB 0 B
build/rich-text/index.js 13.4 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 2.84 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@gziolo gziolo requested a review from sirreal Dec 6, 2020
@jsnajdr jsnajdr force-pushed the webpack5/dependency-extraction-plugin branch from 60aa47d to b7ef25a Dec 7, 2020
@jsnajdr
Copy link
Contributor Author

@jsnajdr jsnajdr commented Dec 7, 2020

@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.

@jsnajdr jsnajdr marked this pull request as ready for review Dec 7, 2020
@jsnajdr jsnajdr requested a review from gziolo as a code owner Dec 7, 2020
@jsnajdr
Copy link
Contributor Author

@jsnajdr jsnajdr commented Dec 7, 2020

Ready for review now 🎉

@gziolo
Copy link
Member

@gziolo gziolo commented Dec 7, 2020

Great work, it's very complex set of changes. It might take me some time to review but it's now on my list 👍

@jsnajdr
Copy link
Contributor Author

@jsnajdr jsnajdr commented Dec 8, 2020

it's very complex set of changes

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.

…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 jsnajdr force-pushed the webpack5/dependency-extraction-plugin branch from b7ef25a to 0fe7332 Dec 8, 2020
@gziolo
gziolo approved these changes Dec 8, 2020
Copy link
Member

@gziolo gziolo left a 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' );

This comment has been minimized.

@gziolo

gziolo Dec 8, 2020
Member

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.

This comment has been minimized.

@jsnajdr

jsnajdr Dec 8, 2020
Author Contributor

Comment added 🙂

jsnajdr added 9 commits Oct 21, 2020
… a package

webpack 5 exposes `webpack-sources` as `webpack.sources` property, and makes sure
that it's exactly the version that you need.
…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 jsnajdr force-pushed the webpack5/dependency-extraction-plugin branch from 0fe7332 to 9453ab2 Dec 8, 2020
@jsnajdr jsnajdr merged commit a35c3fc into master Dec 8, 2020
16 checks passed
16 checks passed
Build Release Artifact
Details
Cancel Previous Runs
Details
Check
Details
build
Details
Admin - 1 Admin - 1
Details
Compare performance with master
Details
pull-request-automation
Details
test (gutenberg-editor-gallery)
Details
test (gutenberg-editor-gallery)
Details
All
Details
JavaScript
Details
Admin - 2 Admin - 2
Details
Admin - 3 Admin - 3
Details
Admin - 4 Admin - 4
Details
PHP
Details
Mobile
Details
@jsnajdr jsnajdr deleted the webpack5/dependency-extraction-plugin branch Dec 8, 2020
@github-actions github-actions bot added this to the Gutenberg 9.6 milestone Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.