-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
WP Scripts: Build block.json viewModule #57461
Conversation
Size Change: 0 B Total Size: 1.69 MB ℹ️ View Unchanged
|
8a38d83
to
99b05ff
Compare
588dba4
to
8e4ea2a
Compare
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
a127bbb
to
42286a3
Compare
42286a3
to
7f850e2
Compare
I spent some time thinking about the problem of colliding assets and I think we should explore changing the output directories of script and module assets. This could mean things like CSS duplication, but I think we're unlikely to have both That should make the cleaning strategies simpler as builds would have more control over their output directories. I think something like this would be good, but I do want to do some testing:
That doesn't need to block review of this PR, it can be landed and we can iterate on the output strategy. |
if ( ! isProduction ) { | ||
// Set default sourcemap mode if it wasn't set by WP_DEVTOOL. | ||
baseConfig.devtool = baseConfig.devtool || 'source-map'; | ||
baseConfig.devServer = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will two configs integrate seamlessly with a single dev server that uses shared setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot, this needs to be configured with different ports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like devServer doesn't really work with multiple compilations: webpack/webpack-cli#2408
I'll only add this to 1 config.
// MiniCSSExtractPlugin to extract the CSS thats gets imported into JavaScript. | ||
new MiniCSSExtractPlugin( { filename: '[name].css' } ), | ||
// React Fast Refresh. | ||
hasReactFastRefresh && new ReactRefreshWebpackPlugin(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think React Fast Refresh will work out of the box for viewModule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may work, but I'm not really sure how to test. Interactivity uses preact, which I don't think is compatible with the react refresh work. I saw this issue about refresh support for preact preactjs/preact#2184
We probably just remove this for now for modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edb3d11
to
a1192fc
Compare
|
I put together a proposal in #57761 that outputs different assets to different directories, but I'm not completely happy with it. I now think that there is less likelihood of assets colliding because assets are named based on files in the src directory. Different assets can't have the same name because they would be the same src file (technically they probably could, but that's unlikely to happen in reality at this time). Let's see what happens while folks use module builds and whether there's a problem in practice. |
@@ -317,52 +380,55 @@ const config = { | |||
process.env.WP_BUNDLE_ANALYZER && new BundleAnalyzerPlugin(), | |||
// MiniCSSExtractPlugin to extract the CSS thats gets imported into JavaScript. | |||
new MiniCSSExtractPlugin( { filename: '[name].css' } ), | |||
// React Fast Refresh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like React Fast Refresh was misplaced. It should remain here for the scripts configuration as it works withe the existing Babel config and the dev server.
It won’t work with the modules config as it’s missing proper Babel handling, the dev server integration. In addition, I’m not sure whether it’s even possible to make it work with Preact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely correct, thanks: #57777
Restore ReactRefreshWebpackPlugin to @wordpress/scripts webpack config. #57461 accidentally moved the plugin to the modules config. It should have remained in the scripts webpack config and not been included in the modules config.
Restore ReactRefreshWebpackPlugin to @wordpress/scripts webpack config. #57461 accidentally moved the plugin to the modules config. It should have remained in the scripts webpack config and not been included in the modules config.
Restore ReactRefreshWebpackPlugin to @wordpress/scripts webpack config. #57461 accidentally moved the plugin to the modules config. It should have remained in the scripts webpack config and not been included in the modules config.
How to change the previously working webpack additional config? I mean I was using something like this to add additional entrypoint:
Now this is not working anymore. I got the error Can you please explain how to handle module compilation with multiple entrypoint? |
const scriptConfig = { | ||
...baseConfig, | ||
|
||
entry: getWebpackEntryPoints( 'script' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be refactored back to a callback function. There are two reasons:
- compatibility with the existing projects that override entry points
- more importantly, entry points are recomputed in the watch mode anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a callback, the signature changed so this now returns the callback:
gutenberg/packages/scripts/utils/config.js
Lines 188 to 202 in 05b6bd7
/** | |
* Detects the list of entry points to use with webpack. There are three ways to do this: | |
* 1. Use the legacy webpack 4 format passed as CLI arguments. | |
* 2. Scan `block.json` files for scripts. | |
* 3. Fallback to `src/index.*` file. | |
* | |
* @see https://webpack.js.org/concepts/entry-points/ | |
* | |
* @param {'script' | 'module'} buildType | |
*/ | |
function getWebpackEntryPoints( buildType ) { | |
/** | |
* @return {Object<string,string>} The list of entry points. | |
*/ | |
return () => { |
I'm investigating the problem now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more importantly, entry points are recomputed in the watch mode anymore
Do you mean if I change block.json (to add another file) when wp-scripts start --experimental-modules
is running, the build doesn't pick it up?
I don't think this has changed at all for script builds or for the script compilation of module builds. I believe the copy webpack plugin is adding the block.json files to the build which makes webpack observe those changes.
Because the modules compilation doesn't include the copy plugins for block.json, when block.json changes the module compilation won't pick it up. I'll look into how to make it observe those changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it should work correctly for scripts when the is no experimental flag provided 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a PR to add block.json
files as webpack file dependencies so they're watched from the module build: #57927
Thanks for the report @Tropicalista. These changes were not intended the modify the webpack config (unless you've opted in to experimental module compilation). I'll investigate. |
I'm sorry, I missed that this was for module compilation, so the build is being run with the @Tropicalista With experimental module compilation we don't support using the webpack configuration directly for extension like in your example. Module compilation via this package is experimental and we need to be able to change or remove the module configuration. In the future, module compilation may not even be handled via webpack. The main issue for discussing this module integration is #57492. Not considering the module webpack config to be public is one of the decisions we made at least for this first version:
|
Restore ReactRefreshWebpackPlugin to @wordpress/scripts webpack config. #57461 accidentally moved the plugin to the modules config. It should have remained in the scripts webpack config and not been included in the modules config.
What?
Add handling of
viewModule
field in block.json when usingwp-scripts
build
orstart
.Part of #57492
Why?
As the Modules API matures, our tooling should support it.
How?
We add an option to clearly mark this an an experimental feature:
wp-scripts build --experimental-modules
.To support modules with webpack, it was necessary to run a multi-compilation. We use an array of webpack configurations instead of a single webpack configuration. This is because module compilation is an option at the compilation level and cannot be set for specific entrypoints.
If the
--experimental-modules
option is found, we use an environment variable to change thewebpack.config
export to return an array[ scriptWebpackConfig, moduleWebpackConfig ]
. Without the experimental option, thewebpack.config
export is the same except for changes that should not impact the script build at all. Consumers should be able to continue extending this config without noticing any differences. I introduced a snapshot test to compare the results, the difference is currently at 20dff00. I did subsequently remove this test as it doesn't seem to have long-term value.Notable changes to the existing webpack config: None.
experiments.outputModule
webpack option is set totrue
. This enables module compilation, but module compilation is controled by other options (output.module
). This should not impact existing builds.As mentioned above, if we find the
WP_EXPERIMENTAL_MODULES
environment variable (set by adding the--experimental-modules
option on the CLI) then the webpack config will export an array of configs.If we run in experimental modules mode, the
CleanWebpackPlugin
could remove assets that should be included. This is because both compilations output to the same output directory. Ideally, each compilation would have control over its output directory and not share them. That's difficult and I don't have a good solution at this time given that we do want to share other assets like stylesheets. Additionally, if we were to output to different directories, the block.jsonfile:…
rewriting would need to account for that.I don't have a convincing solution for either using multiple directories or preventing assets from being clobbered at this time. Given that this is clearly flagged as experimental, I believe that's acceptable and we can improve things if a convincing solution presents itself.
Since this is experimental, I wanted avoided adding more webpack configuration files. I don't want to expose more files that could folks require and extend. We could consider adding an
exports
field to this package to control access to certain things, but that's likely a breaking change I didn't want to undertake here.Testing Instructions
To test this, you'll want to check this out and link a project that depends on
@wordpress/scripts
to use this PR.If you use
wp-scripts build
orwp-scripts start
, there should be no significant changes with the current behavior.You can test the experimental build by adding a
viewModule
field to block.json in a project, then running a script likewp-scripts -- build --experimental-modules
to build it.I've been running the experimental build of https://github.com/sirreal/wp-scripts-interactivity-module-demo (the packages is linked to my development copy locally). The build produces the expected results with both scripts and modules. With #57437 (handling
viewModule
in block registration), this results in a working plugin. Here's the build ouptut:block.json
index.asset.php
index.js
render.php
view.asset.php
view.js