Build: Prepare build for more script modules #65064
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
|
Size Change: +288 B (+0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 0a71b7d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10808765400
|
gziolo
added
the
[Feature] Script Modules API
gziolo
reviewed
Sep 4, 2024
gziolo
reviewed
Sep 5, 2024
tools/webpack/script-modules.js
Outdated
| 'interactivity/debug': './packages/interactivity/src/debug', | ||
| 'interactivity/router': './packages/interactivity-router', | ||
|
|
||
| 'block-library/file': './packages/block-library/src/file/view.js', |
There was a problem hiding this comment.
Sharing my notes: glob for view files would help here with maintenance. Feel free to ignore them as I need to collect some ideas next to the code I see.
packages/interactivity/package.json
Outdated
| @@ -24,6 +24,7 @@ | |||
| }, | |||
| "main": "build/index.js", | |||
| "module": "build-module/index.js", | |||
| "wp-module": "build-module/index.js", | |||
There was a problem hiding this comment.
I was thinking about how we could smoothly transition to ES Modules here. What if we start using "type": "module" and completely remove the obsolete support for CJS here. This way we could easily distinguish what the package build process should do about packages:
"type": "module" + exports - ES Module only
"main": "some/path.js" - CJS
"module": "some/path.js" - ES Module
More on package entry points here. I'm still processing it all, so this is more of me documenting the efforts.
There was a problem hiding this comment.
Interesting fact, we use module field when deciding whether to build the package:
gutenberg/bin/packages/get-packages.js
Line 45 in e40447e
There was a problem hiding this comment.
We never check whether CJS is required or not, but we only generate the folder in the production mode to speed up npm run dev as CJS isn't consumed by the Gutenberg plugin:
gutenberg/bin/packages/build-worker.js
Lines 32 to 37 in e40447e
|
@sirreal, you started looking at how to convert more packages to ESM, so I spent some time investigating how we could annotate such packages in a way where webpack could easily distinguish that from package.json files by using existing fields like |
|
@gziolo I had explored that in #60952 and used the #65101 implements a |
This was referenced Sep 9, 2024
sirreal
force-pushed
the
update/script-modules-build
branch
2 times, most recently
from
2c1ce1a to
4552a8a
Compare
gziolo
reviewed
Sep 10, 2024
| @@ -30,6 +30,13 @@ | |||
| "src/**/*.scss", | |||
| "{src,build,build-module}/*/init.js" | |||
| ], | |||
| "wp-script-module-exports": { | |||
| "./file/view": "./build-module/file/view.js", | |||
There was a problem hiding this comment.
It starts to look like exports from package.json. Nice! 👍🏻
There was a problem hiding this comment.
Yes! I think this is going to be a very good direction. It moves in the direction of exports, but doesn't switch all the packages immediately.
It's also a custom field, and these are custom builds in general specific for use as WordPress Script Modules. It makes it clear that they're not intended for general consumption.
There was a problem hiding this comment.
I also think we can move in the direction of generating the entrypoints in a standard way by reading package.json files.
There was a problem hiding this comment.
Unfortunately, it doesn't work 🙃 (I thought it did but I'm not sure why that was).
Problems:
When entrypoints are paths (./packages/block-library/file/view), webpack never sees the package.json in order to find these paths.
ERROR in block-library/file/view
Module not found: Error: Can't resolve './packages/block-library/file/view' in '…'
When entrypoints are to modules (@wordpress/block-library/file/view) webpack resolves these correctly, but they're handled as externals. DEWP rejects these. We could fix DEWP, but even then we have entrypoints like @wordpress/interactivity which DEWP will externalize and there doesn't seem to be a good way to say "Externalize this except when it's the entrypoint." Externalizing entrypoints is, needless to say, bad and broken 🙂
So I don't think actually using this field as webpack exportsFields is going to work, but we can read this field and build a path to it programatically.
There was a problem hiding this comment.
So I don't think actually using this field as webpack exportsFields is going to work, but we can read this field and build a path to it programatically.
That sounds like a great first step with some thin compact layer in the webpack config that scans package.json files and produces entry points that are hardcoded today. For blocks you could eventually use pattern matching.
sirreal
added
[Type] Build Tooling
|
I've added the "no core sync required" label. This should not impact Core right now, although these changes would be ported to Core in the future to support building modules correctly. |
sirreal
commented
Sep 10, 2024
| @@ -46,6 +46,7 @@ | |||
| "@wordpress/i18n": "file:packages/i18n", | |||
| "@wordpress/icons": "file:packages/icons", | |||
| "@wordpress/interactivity": "file:packages/interactivity", | |||
| "@wordpress/interactivity-router": "file:packages/interactivity-router", | |||
There was a problem hiding this comment.
This was omitted but should have been present.
sirreal
requested review from
spacedmonkey,
desrosj,
luisherranz,
DAreRodz,
artemiomorales and
ajitbohra
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I would like to keep that work separate as it's preexisting and on its own it will be easier to work with. I will look at build-plugin-zip, the list of |
gziolo
reviewed
Sep 11, 2024
| @@ -98,6 +98,7 @@ zip -r gutenberg.zip \ | |||
| packages/block-serialization-default-parser/*.php \ | |||
| post-content.php \ | |||
| $build_files \ | |||
| build-module \ | |||
There was a problem hiding this comment.
I don't remember why there was pattern matching used for files in the build folder. @youknowriad, is there anything important we might miss when including the entire build-module? At the moment, we want all files to be included from there.
There was a problem hiding this comment.
I created related PR #65064
gziolo
approved these changes
Sep 11, 2024
There was a problem hiding this comment.
Code looks great, I spent some time testing both npm run dev and npm run build. Everything is working as before. I confirmed that the key Interactivity API features: the lightbox in the Image block and instant pagination in the Query block work correctly. I also tested with the Playground instance that confirms that changes to the zip file provided for the plugin is fully functional.
There are some open questions to resolve, and we need to confirm that everything works correctly on Windows before proceeding. I'm approving, as the rest is not under my control.
|
I checked out this branch on my Windows host OS and ran
However, I noticed that the PHP files for the blocks are not generated in the
Because I cannot build the PHP files for the blocks changed in this PR, I cannot test the behavior of the Image block and other blocks on the front end. However, this problem also occurs in the trunk. I have not yet been able to investigate this in detail, but it might be related to #63098. In any case, the |
|
Here's the expected output of listing |
|
Thanks @t-hamano! It looks like this is OK to move ahead if it's building on Windows 👏 |
2 tasks
I have reported this issue in detail in #65244. I will investigate the cause. |
|
Opened a follow-up PR to clean up the unused legacy scripts for core blocks: |
This was referenced Sep 19, 2024
sirreal
added a commit
that referenced
this pull request
Sep 20, 2024
Rework how Script Modules are registered in Gutenberg. Script Module registration is handled in one central place. A combined assets file is used for Script Modules and registration. This means that dependencies and versions will be used correctly and kept up-to-date while avoiding repeated file reads. Block library Script Module assets that are enqueued on demand _are registered in a centralized location_. The assets are enqueued on demand. **This requires a Core change** since the block library PHP files are synced to Core and also require centralized Script Module registration (WordPress/wordpress-develop#7360). This solves a problem where Gutenberg-specific code was being shipped in Core through block-library. The block library Script Module asset Module IDs are renamed to indicate they are view files and align with the naming from #65064: @wordpress/block-library/query is @wordpress/block-library/query/view (indicating it is a view file). --- This is sufficient to change Script Modules to use Gutenberg in a backwards compatible way: - `@wordpress/ineractivity` and `@wordpress/interactivity-router` were registered on `wp_enqueue_scripts`. That action fires after the `wp_default_scripts` used here. Registering an already registered Script Module is a no-op. This change registers first. - The only other Script Modules currently available in Core are from the block library. Those have been registered conditionally on use. The ID is changed here, so there's little risk of the wrong version being used. There is a Core companion PR that will be necessary to land: WordPress/wordpress-develop#7360 --- Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]>
gutenbergplugin
pushed a commit
that referenced
this pull request
Sep 20, 2024
Rework how Script Modules are registered in Gutenberg. Script Module registration is handled in one central place. A combined assets file is used for Script Modules and registration. This means that dependencies and versions will be used correctly and kept up-to-date while avoiding repeated file reads. Block library Script Module assets that are enqueued on demand _are registered in a centralized location_. The assets are enqueued on demand. **This requires a Core change** since the block library PHP files are synced to Core and also require centralized Script Module registration (WordPress/wordpress-develop#7360). This solves a problem where Gutenberg-specific code was being shipped in Core through block-library. The block library Script Module asset Module IDs are renamed to indicate they are view files and align with the naming from #65064: @wordpress/block-library/query is @wordpress/block-library/query/view (indicating it is a view file). --- This is sufficient to change Script Modules to use Gutenberg in a backwards compatible way: - `@wordpress/ineractivity` and `@wordpress/interactivity-router` were registered on `wp_enqueue_scripts`. That action fires after the `wp_default_scripts` used here. Registering an already registered Script Module is a no-op. This change registers first. - The only other Script Modules currently available in Core are from the block library. Those have been registered conditionally on use. The ID is changed here, so there's little risk of the wrong version being used. There is a Core companion PR that will be necessary to land: WordPress/wordpress-develop#7360 --- Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]>
sirreal
added a commit
that referenced
this pull request
Oct 25, 2024
Document recent changes to the build system that change are requirements for publishing WordPress scripts and script modules. #65064 updated the way that script modules from packages are bundled for Gutenberg and WordPress. #66272 updated the way that scripts from packages are bundled for Gutenberg and WordPress. --------- Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: justintadlock <[email protected]> Co-authored-by: youknowriad <[email protected]>
|
Linking interesting follow-up conversation: #66428 (comment) |
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…ress#66428) Document recent changes to the build system that change are requirements for publishing WordPress scripts and script modules. WordPress#65064 updated the way that script modules from packages are bundled for Gutenberg and WordPress. WordPress#66272 updated the way that scripts from packages are bundled for Gutenberg and WordPress. --------- Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: justintadlock <[email protected]> Co-authored-by: youknowriad <[email protected]>
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?
Prepare webpack module build to handle more Script Module builds for use in WordPress.
build-modulefolder (adjacent to thebuildfolder currently used for scripts).wpScriptModulesExportsfield and configure webpack to use it. This follows the same basic syntax as package.jsonexportsfields. Multiple module entrypoints can be exposed per package. However, it remains a custom field, so it is clear that these entrypoints are not intended for general consumption. In the future, module-only packages (interactivity, interactivity-router) can switch to using exports directly and likely addtype: module, but that's follow-up work 🙂wpScriptModulesExportsdirectly, so packages are inspected programmatically in order to generated webpack script modules entrypoints.This is a first step towards building more script modules, such as
@wordpress/a11ywhich may be used in #62906.#65101 shows how this is used to build the
@wordpress/a11yscript module.Related PRs:
Why?
This prepares the build to produce more script modules.
How?
See above.
Testing Instructions
It's important to ensure that script modules are correctly registered. This should be covered by e2e tests, but testing, in particular of the affected blocks (image, query, etc.) on the frontend is appreciated.