DependencyExtractionWebpackPlugin: Throw when using scripts from modules #57795
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: +207 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
sirreal
force-pushed
the
add/dewp-throw-on-modules-using-scripts
branch
from
75cf701
to
e4f147e
Compare
sirreal
commented
Jan 12, 2024
| @@ -12,7 +12,9 @@ module.exports = { | |||
| new DependencyExtractionWebpackPlugin( { | |||
| combineAssets: true, | |||
| requestToExternalModule( request ) { | |||
| return request.startsWith( '@wordpress/' ); | |||
| return ( | |||
| request.startsWith( '@wordpress/' ) || request === 'lodash' | |||
There was a problem hiding this comment.
In the module tests lodash was not externalized but bundled. Now it is externalized (to prevent errors). This is reflected in the snapshots.
There was a problem hiding this comment.
I noticed in the original PR that lodash gets bundled into the fixture's build output, but I thought it's desired 🙂 Also, the tests don't look at the build output at all, only at the dependency metadata.
sirreal
commented
Jan 12, 2024
Comment on lines
93
to
94
| if ( externalRequest instanceof Error ) { | ||
| return callback( externalRequest ); |
There was a problem hiding this comment.
It's best not to throw the error directly. If we throw it directly webpack doesn't know where the error is coming from and shows a general error.
throw:
ERROR in Attempted to use WordPress script in a module: @wordpress/api-fetch
callback( err ):
ERROR in ./src/index.js 2:0-38
Module not found: Error: Attempted to use WordPress script in a module: @wordpress/api-fetch
Another option would be to throw the error in defaultRequestToExternal and use try/catch to pass the error to the callback.
sirreal
added
[Type] Task
jsnajdr
reviewed
Jan 12, 2024
| return new Error( | ||
| `Attempted to use WordPress script in a module: ${ request }` | ||
| ); | ||
| } |
There was a problem hiding this comment.
If we want to prevent bundling @wordpress/* modules by accident, then the defaultRequestToExternalModule should externalize everything @wordpress/* by default. It's then the user's responsibility to add them to the importmap, otherwise the runtime import '@wordpress/x' will fail.
That's an approach identical to scripts, only instead of ensuring that scripts are wp_enqueued we need to ensure that modules are in importmap.
There was a problem hiding this comment.
If we really decide to treat @wordpress/* imports as an error by default, it's better to throw the error rather than return it.
There was a problem hiding this comment.
It's then the user's responsibility to add them to the importmap, otherwise the runtime import '@wordpress/x' will fail
I think we're not there yet, as there are no @wordpress packages available as modules except for @wordpress/interactivity.
There was a problem hiding this comment.
My understanding is that we can't use scripts from modules at all at this time. Adding them to the import map wouldn't help because they don't export anything, they're not modules.
We can throw the error here, that's fine. We'll need to try/catch and pass it to the callback in the plugin (https://github.com/WordPress/gutenberg/pull/57795/files#r1450176826)
There was a problem hiding this comment.
My understanding is that we can't use scripts from modules at all at this time.
But even without this PR we will never use any actual WP script anyway. We'll either bundle a local node_modules package, or output an import of something that's expected to resolve to a ES module.
If we keep this, it feels intuitively wrong to me to piggy-pack on defaultRequestToExternal. If we want to forbid @wordpress/*, write it explicitly in defaultRequestToExternalModule. Also, the error message will be very confusing if it prevents an import of lodash, react or moment. These are common NPM libraries, not WordPress-specific.
There was a problem hiding this comment.
I share some of your reservations about this change. But I think that it's a good idea to provide this kind of feedback because during this scripts<->modules transition there's likely to be a lot of confusion and mistakes. This kind of change is intended to bring some clarity at an ideal time. It would help a lot of there were a post we could link to in the error message about the plans for this phase. @luisherranz is there anything public about scripts-modules compatibility we could link to?
This plugin is specific to WordPress. It can be used outside of WordPress, but that's not its primary purpose. If we do use it outside of WordPress, we should set useDefaults: false and we wouldn't get any errors.
I piggy-backed on defaultRequestToExternal because the idea here is that if you're trying to use those scripts when compiling a Module for Wordpress, you're probably doing it wrong (at this time). defaultRequestToExternal is a pretty good approximation of what scripts are available and what we're likely to see used. There may be legacy scripts that will never be available as modules and maybe we should maintain a list of those going forward.
Some of those other modules probably work - react, lodash, moment - but I'd hate to have folks compiling a frontend module for multiple blocks and bundling those three dependencies in each one.
There was a problem hiding this comment.
Alright, please go forward then 🙂 I don't have any other feedback.
| @@ -12,7 +12,9 @@ module.exports = { | |||
| new DependencyExtractionWebpackPlugin( { | |||
| combineAssets: true, | |||
| requestToExternalModule( request ) { | |||
| return request.startsWith( '@wordpress/' ); | |||
| return ( | |||
| request.startsWith( '@wordpress/' ) || request === 'lodash' | |||
There was a problem hiding this comment.
I noticed in the original PR that lodash gets bundled into the fixture's build output, but I thought it's desired 🙂 Also, the tests don't look at the build output at all, only at the dependency metadata.
sirreal
commented
Jan 12, 2024
Comment on lines
71
to
77
| // requestToExternalModule allows a boolean shorthand | ||
| if ( externalRequest === false ) { | ||
| externalRequest = undefined; | ||
| } | ||
| if ( externalRequest === true ) { | ||
| externalRequest = request; | ||
| } |
There was a problem hiding this comment.
This fixes a small bug where we wouldn't run our default if we returned false (only available with modules).
luisherranz
added
the
Backport to Gutenberg Minor Release
luisherranz
approved these changes
Jan 12, 2024
There was a problem hiding this comment.
There is a chance that people will get stuck using an old version of wp-scripts when they are using a newer version of Gutenberg/WordPress that has support for modularized @wordpress packages and this will incorrectly throw, but it's true that without throwing it will still fail because the current version doesn't those packages as externals either.
So, up to you to decide if this is a good idea or not, but it's fine on my side.
Co-authored-by: Luis Herranz <luisherranz@gmail.com>
sirreal
removed
the
Backport to Gutenberg Minor Release
|
I'm removing the backport label @luisherranz. Since this is tooling I don't think it needs to be backported, just released. The packages were never released with 17.5 so that needs to happen too. |
|
Hmm... but without the backport label they won't be included in the
So anything that it's not in the EDIT: Oh, I think I added the wrong label at first. It should be |
sirreal
added
Backport to Gutenberg Minor Release
|
I've added the labels again. |
cbravobernal
removed
the
Backport to Gutenberg Minor Release
|
There is no need a backport to Gutenberg Minor label, just to Gutenberg RC, as we are between 17.5RC and 17.5. We need to change also the milestone 😄 |
|
And as the packages were not released, we may force a release with this backport. In case is not cherry picked the release lead can be pinged. |
sirreal
added a commit
that referenced
this pull request
Jan 15, 2024
…les (#57795) WordPress Script dependencies are not currently available as dependencies of WordPress Modules. Using e.g. lodash or @wordpress/api-fetch in a module build would result in bundling that dependency. For a package like lodash that's undesirable but should work. However, many @wordpress/* packages are not intended to be bundle or duplicated and will not work as expected. It's likely an error to use WordPress Scripts inside modules at this time. --------- Co-authored-by: Luis Herranz <luisherranz@gmail.com>
gziolo
pushed a commit
that referenced
this pull request
Jan 16, 2024
…les (#57795) WordPress Script dependencies are not currently available as dependencies of WordPress Modules. Using e.g. lodash or @wordpress/api-fetch in a module build would result in bundling that dependency. For a package like lodash that's undesirable but should work. However, many @wordpress/* packages are not intended to be bundle or duplicated and will not work as expected. It's likely an error to use WordPress Scripts inside modules at this time. --------- Co-authored-by: Luis Herranz <luisherranz@gmail.com>
priethor
added
Backport to Gutenberg RC
scruffian
pushed a commit
that referenced
this pull request
Jan 17, 2024
…les (#57795) WordPress Script dependencies are not currently available as dependencies of WordPress Modules. Using e.g. lodash or @wordpress/api-fetch in a module build would result in bundling that dependency. For a package like lodash that's undesirable but should work. However, many @wordpress/* packages are not intended to be bundle or duplicated and will not work as expected. It's likely an error to use WordPress Scripts inside modules at this time. --------- Co-authored-by: Luis Herranz <luisherranz@gmail.com>
|
I just cherry-picked this PR to the release/17.5 branch to get it included in the next release: 9c69a6b |
scruffian
removed
the
Backport to Gutenberg RC
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?
Throw errors when using WordPress script dependencies in a module build.
Part of #57492
Concerns
I'm requesting review but I have some concerns. This was discussed as part of #57492 and it seems important to address what we expect to be a common error. I do have concerns about this approach.
Scripts are not currently supported, but we hope to support scripts soon. Is this a good solution? Should there be an option? When and how will we remove the error throwing from this plugin?
Why?
WordPress Script dependencies are not currently available as dependencies of WordPress Modules. Using e.g.
lodashor@wordpress/api-fetchin a module build would result in bundling that dependency. For a package likelodashthat's undesirable but should work. However, many@wordpress/*packages are not intended to be bundle or duplicated and will not work as expected.It's likely an error to use WordPress Scripts inside modules at this time.
How?
If the dependency extraction plugin detects a Script dependency inside a Module build, it will throw an error.
Testing Instructions
This is tested on CI.
You can try linking and using the plugin in a local build. If script dependencies are found it should error and not complete the build: