Dependency Extraction Webpack Plugin: Add Module support #57199
Merged
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
gziolo
added
[Package] Dependency Extraction Webpack Plugin
sirreal
added
the
[Feature] Script Modules API
|
Flaky tests detected in 0b09434. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7409346640
|
sirreal
force-pushed
the
add/dewp-esmodules
branch
from
fac82f9
to
96b54b0
Compare
|
Size Change: +69 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
sirreal
commented
Dec 21, 2023
sirreal
added a commit
that referenced
this pull request
Dec 22, 2023
sirreal
added a commit
that referenced
this pull request
Dec 22, 2023
Webpack errors were not correctly handled the tests. Webpack does not always pass an error to the callback, instead it provides errors in the compilation stats. Here, we check for those errors correctly and tests fail if webpack completes with errors. Some configuration and test names are also updated in preparation for #57199.
sirreal
force-pushed
the
add/dewp-esmodules
branch
3 times, most recently
from
efff4ab
to
6c194f0
Compare
sirreal
changed the title
sirreal
commented
Dec 22, 2023
sirreal
commented
Dec 22, 2023
packages/dependency-extraction-webpack-plugin/test/fixtures/wordpress-interactivity/index.js
Outdated
Show resolved
Hide resolved
sirreal
force-pushed
the
add/dewp-esmodules
branch
from
bb38560
to
4924a9b
Compare
sirreal
commented
Dec 27, 2023
sirreal
force-pushed
the
add/dewp-esmodules
branch
from
2b6aeb2
to
0ce6c61
Compare
sirreal
commented
Jan 2, 2024
Comment on lines
+299
to
+292
| if ( this.useModules ) { | ||
| assetData.type = 'module'; | ||
| } |
There was a problem hiding this comment.
It's not necessary to add this at the moment, but it may be helpful to know this is a module asset file.
There was a problem hiding this comment.
Would it make sense to reuse register_block_script_handle for the module type and branch code depending on this setting in the asset data?
There was a problem hiding this comment.
That's exactly the kind of thing where {"type": "module"} would be helpful 👍
I'm not aware of any Modules work that mentions changes to the assets file (aside from the changes to the dependencies array implied by WordPress/wordpress-develop#5818 and #57231). The only work I'm aware of there is #57437.
There was a problem hiding this comment.
Do you know if there's a specification of asset files somewhere? I want to make sure we update that accordingly.
sirreal
force-pushed
the
add/dewp-esmodules
branch
from
7ab54b0
to
256684a
Compare
Co-authored-by: Luis Herranz <luisherranz@gmail.com>
sirreal
force-pushed
the
add/dewp-esmodules
branch
from
41634ed
to
0b09434
Compare
|
Thanks for flagging that issue with the dynamic checking @luisherranz! I pushed some changes to fix the static/dynamic check for dependencies. For a dependency to be static, we should be able to trace static imports from the entrypoint to the module. If that sequence is ever broken by a dynamic import, the dependency is dynamic. This involved module graph traversal. I added some tests for circular dependencies because I was a bit worried we could get into an infinite loop, but there's no issue with the tests I added. |
jsnajdr
reviewed
Jan 4, 2024
|
|
||
| **Note:** This plugin overlaps with the functionality provided by [webpack `externals`](https://webpack.js.org/configuration/externals). This plugin is intended to extract module handles from bundle compilation so that a list of module dependencies does not need to be manually maintained. If you don't need to extract a list of module dependencies, use the `externals` option directly. | ||
|
|
||
| This plugin is compatible with `externals`, but they may conflict. For example, adding `{ externals: { '@wordpress/blob': 'wp.blob' } }` to webpack configuration will effectively hide the `@wordpress/blob` module from the plugin and it will not be included in dependency lists. |
There was a problem hiding this comment.
It's more precise to say not that the plugin "overlaps" with externals, but that it actually uses the externals plugin internally! It creates a configuration for it and passes it to the externals plugin to do the actual work.
A conflict may arise when you create another instance of the externals plugin in your config, and they both want to process the same module. Then only one can win.
The other, fairly independent functionality of DEWP is finding all the external imports in the bundle and recording them in the .asset.php file.
The way how both functionalities are mixed together can be confusing for users who are trying to understand what DEWP does. We discussed that once in 2022 with @anomiex but never took any real action to fix it.
There was a problem hiding this comment.
This part is copied from the "behavior with scripts" section. There's certainly nuance here, but externals in this case refers to externals as a configuration option (which I believe uses an ExternalPlugin under the hood, but that's not apparent via webpack config).. The goal is to make it clear that you really shouldn't be adding your own externals declarations for WordPress script/module dependencies.
If you have concrete suggestions for how to improve this (and the text in the other section) I'm happy to make changes.
There was a problem hiding this comment.
If you have concrete suggestions for how to improve this
I think I'll do my own PR with a docs update, especially now when this one is merged 👍
Avoid instantiating with one externals type and switching later
luisherranz
approved these changes
Jan 4, 2024
gziolo
reviewed
Jan 4, 2024
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.
What?
Add support for extracting modules information with the Dependency Extraction Webpack Plugin.
Part of #57492
Why?
As the modules support matures, we should provide at least the same experience folks currently have with scripts.
How?
First: Behavior with scripts remains unchanged. Folks can continue to use this plugin in exactly the same way without any changes. These changes should be low risk and the module behavior can be considered experimental. Should we add a note about that in the README?
We add the necessary functionality to this plugin to support modules.
To determine whether to compile in module or script mode, we check the webpack option
output.modules. If this option is true, the plugin will compile for modules.A new option is added
requestToExternalModule, it serves the same purpose asrequestToExternalfor scripts. The same option could have been reused, but I created a new option to avoid confusion.One notable difference is that, we extract information about whether a module is dynamic
const x = await import( … )or staticimport x from …sowp_register_modulecan correctly list static and dynamic module dependencies.Testing Instructions
The test suite has been adapted to produce module output. You can check the snapshots added in the PR.