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.

Sign up for GitHub

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

DependencyExtractionWebpackPlugin: Throw when using scripts from modules #57795

Merged
merged 6 commits into from
Jan 12, 2024
Merged

DependencyExtractionWebpackPlugin: Throw when using scripts from modules #57795

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4ba4632
Implement error throwing on Scripts imported from Modules
sirreal Jan 12, 2024
e018681
Update snapshots
sirreal Jan 12, 2024
e4f147e
Improve requestToExternalModule shorthand handling
sirreal Jan 12, 2024
602162b
Throw and handle errors from requestToExternal
sirreal Jan 12, 2024
5860e6a
Add "not supported yet" to error message
sirreal Jan 12, 2024
6d26ec1
update snapshots
sirreal Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/dependency-extraction-webpack-plugin/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ class DependencyExtractionWebpackPlugin {
if ( typeof this.options.requestToExternalModule === 'function' ) {
externalRequest =
this.options.requestToExternalModule( request );

// requestToExternalModule allows a boolean shorthand
if ( externalRequest === false ) {
externalRequest = undefined;
}
if ( externalRequest === true ) {
externalRequest = request;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a small bug where we wouldn't run our default if we returned false (only available with modules).

}
} else if ( typeof this.options.requestToExternal === 'function' ) {
externalRequest = this.options.requestToExternal( request );
Expand All @@ -82,8 +90,8 @@ class DependencyExtractionWebpackPlugin {
: defaultRequestToExternal( request );
}

if ( this.useModules && externalRequest === true ) {
externalRequest = request;
if ( externalRequest instanceof Error ) {
return callback( externalRequest );
Copy link
Member Author

@sirreal sirreal Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if ( externalRequest ) {
Expand Down
16 changes: 14 additions & 2 deletions packages/dependency-extraction-webpack-plugin/lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,13 @@ function defaultRequestToExternal( request ) {
*
* Currently only @wordpress/interactivity
*
* Do not use the boolean shorthand here, it's only handled for the `requestToExternalModule` option.
*
* @param {string} request Module request (the module name in `import from`) to be transformed
* @return {string|undefined} The resulting external definition. Return `undefined`
* to ignore the request. Return `string` to map the request to an external. This may simply be returning the request, e.g. `@wordpress/interactivity` maps to the external `@wordpress/interactivity`.
* @return {string|Error|undefined} The resulting external definition.
* - Return `undefined` to ignore the request (do not externalize).
* - Return `string` to map the request to an external.
* - Return `Error` to emit an error.
*/
function defaultRequestToExternalModule( request ) {
if ( request === '@wordpress/interactivity' ) {
Expand All @@ -73,6 +77,14 @@ function defaultRequestToExternalModule( request ) {
// which forces @wordpress/interactivity imports to be hoisted to static imports.
return `module ${ request }`;
}

const isWordPressScript = Boolean( defaultRequestToExternal( request ) );

if ( isWordPressScript ) {
return new Error(
`Attempted to use WordPress script in a module: ${ request }`
sirreal marked this conversation as resolved.
Show resolved Hide resolved
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really decide to treat @wordpress/* imports as an error by default, it's better to throw the error rather than return it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, please go forward then 🙂 I don't have any other feedback.

}

/**
Expand Down
99 changes: 86 additions & 13 deletions packages/dependency-extraction-webpack-plugin/test/__snapshots__/build.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`DependencyExtractionWebpackPlugin modules Webpack \`combine-assets\` should produce expected output: Asset file 'assets.php' should match snapshot 1`] = `
"<?php return array('fileA.mjs' => array('dependencies' => array('@wordpress/blob'), 'version' => 'b2c5cea79a32b3d91bf8', 'type' => 'module'), 'fileB.mjs' => array('dependencies' => array('@wordpress/token-list'), 'version' => '5c4197fd48811f25807f', 'type' => 'module'));
"<?php return array('fileA.mjs' => array('dependencies' => array('@wordpress/blob', 'lodash'), 'version' => '4ab8cc4b6b7619053443', 'type' => 'module'), 'fileB.mjs' => array('dependencies' => array('@wordpress/token-list'), 'version' => '5c4197fd48811f25807f', 'type' => 'module'));
"
`;

Expand All @@ -17,6 +17,11 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`combine-assets\` sh
"request": "@wordpress/token-list",
"userRequest": "@wordpress/token-list",
},
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
]
`;

Expand Down Expand Up @@ -65,8 +70,17 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`dynamic-import\` sh
]
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`error\` should produce expected output 1`] = `
"ERROR in ./index.js 5:0-23
Module not found: Error: Attempted to use WordPress script in a module: jquery

ERROR in ./index.js 6:23-55
Module not found: Error: Attempted to use WordPress script in a module: @wordpress/api-fetch
"
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`function-output-filename\` should produce expected output: Asset file 'chunk--main--main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('@wordpress/blob'), 'version' => '2925e30449840a5a80f8', 'type' => 'module');
"<?php return array('dependencies' => array('@wordpress/blob', 'lodash'), 'version' => 'e325da3aa7dbb0c0c151', 'type' => 'module');
"
`;

Expand All @@ -77,11 +91,16 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`function-output-fil
"request": "@wordpress/blob",
"userRequest": "@wordpress/blob",
},
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
]
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`has-extension-suffix\` should produce expected output: Asset file 'index.min.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('@wordpress/blob'), 'version' => '26d6da43027f3522b0ca', 'type' => 'module');
"<?php return array('dependencies' => array('@wordpress/blob', 'lodash'), 'version' => '8308f1ac78c21f09c721', 'type' => 'module');
"
`;

Expand All @@ -92,6 +111,11 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`has-extension-suffi
"request": "@wordpress/blob",
"userRequest": "@wordpress/blob",
},
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
]
`;

Expand Down Expand Up @@ -130,7 +154,7 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`no-deps\` should pr
exports[`DependencyExtractionWebpackPlugin modules Webpack \`no-deps\` should produce expected output: External modules should match snapshot 1`] = `[]`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`option-function-output-filename\` should produce expected output: Asset file 'chunk--main--main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('@wordpress/blob'), 'version' => '2925e30449840a5a80f8', 'type' => 'module');
"<?php return array('dependencies' => array('@wordpress/blob', 'lodash'), 'version' => 'e325da3aa7dbb0c0c151', 'type' => 'module');
"
`;

Expand All @@ -141,11 +165,16 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`option-function-out
"request": "@wordpress/blob",
"userRequest": "@wordpress/blob",
},
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
]
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`option-output-filename\` should produce expected output: Asset file 'main-foo.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('@wordpress/blob'), 'version' => '2925e30449840a5a80f8', 'type' => 'module');
"<?php return array('dependencies' => array('@wordpress/blob', 'lodash'), 'version' => 'e325da3aa7dbb0c0c151', 'type' => 'module');
"
`;

Expand All @@ -156,12 +185,25 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`option-output-filen
"request": "@wordpress/blob",
"userRequest": "@wordpress/blob",
},
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
]
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`output-format-json\` should produce expected output: Asset file 'main.asset.json' should match snapshot 1`] = `"{"dependencies":[],"version":"34504aa793c63cd3d73a","type":"module"}"`;
exports[`DependencyExtractionWebpackPlugin modules Webpack \`output-format-json\` should produce expected output: Asset file 'main.asset.json' should match snapshot 1`] = `"{"dependencies":["lodash"],"version":"4e62c01516f9dab8041f","type":"module"}"`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`output-format-json\` should produce expected output: External modules should match snapshot 1`] = `[]`;
exports[`DependencyExtractionWebpackPlugin modules Webpack \`output-format-json\` should produce expected output: External modules should match snapshot 1`] = `
[
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
]
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`overrides\` should produce expected output: Asset file 'main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('@wordpress/blob', '@wordpress/url', 'rxjs', 'rxjs/operators'), 'version' => '259fc706528651fc00c1', 'type' => 'module');
Expand Down Expand Up @@ -199,12 +241,12 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`runtime-chunk-singl
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`runtime-chunk-single\` should produce expected output: Asset file 'b.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('@wordpress/blob'), 'version' => 'a0ec8ef279476bb51e19', 'type' => 'module');
"<?php return array('dependencies' => array('@wordpress/blob', 'lodash'), 'version' => 'e14dfa7260edaee86a85', 'type' => 'module');
"
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`runtime-chunk-single\` should produce expected output: Asset file 'runtime.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array(), 'version' => '0bb8a9fae3dcfcc1ac38', 'type' => 'module');
"<?php return array('dependencies' => array(), 'version' => 'e7402f5608a888d8fd66', 'type' => 'module');
"
`;

Expand All @@ -215,11 +257,16 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`runtime-chunk-singl
"request": "@wordpress/blob",
"userRequest": "@wordpress/blob",
},
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
]
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`style-imports\` should produce expected output: Asset file 'main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('@wordpress/blob'), 'version' => '38479966fb62d588f05e', 'type' => 'module');
"<?php return array('dependencies' => array('@wordpress/blob', 'lodash'), 'version' => '22d18a3461df47fbaa79', 'type' => 'module');
"
`;

Expand All @@ -230,11 +277,16 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`style-imports\` sho
"request": "@wordpress/blob",
"userRequest": "@wordpress/blob",
},
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
]
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`wordpress\` should produce expected output: Asset file 'main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('@wordpress/blob'), 'version' => '2925e30449840a5a80f8', 'type' => 'module');
"<?php return array('dependencies' => array('@wordpress/blob', 'lodash'), 'version' => 'e325da3aa7dbb0c0c151', 'type' => 'module');
"
`;

Expand All @@ -245,16 +297,26 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`wordpress\` should
"request": "@wordpress/blob",
"userRequest": "@wordpress/blob",
},
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
]
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`wordpress-interactivity\` should produce expected output: Asset file 'main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array(array('id' => '@wordpress/interactivity', 'type' => 'dynamic')), 'version' => 'f0242eb6da78af6ca4b8', 'type' => 'module');
"<?php return array('dependencies' => array('lodash', array('id' => '@wordpress/interactivity', 'type' => 'dynamic')), 'version' => 'fcc07ce68574cdc2a6a5', 'type' => 'module');
"
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`wordpress-interactivity\` should produce expected output: External modules should match snapshot 1`] = `
[
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
{
"externalType": "module",
"request": "@wordpress/interactivity",
Expand All @@ -264,7 +326,7 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`wordpress-interacti
`;

exports[`DependencyExtractionWebpackPlugin modules Webpack \`wordpress-require\` should produce expected output: Asset file 'main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('@wordpress/blob'), 'version' => '52c1849898b74d94f025', 'type' => 'module');
"<?php return array('dependencies' => array('@wordpress/blob', 'lodash'), 'version' => 'f40de15d66b54da440d2', 'type' => 'module');
"
`;

Expand All @@ -275,6 +337,11 @@ exports[`DependencyExtractionWebpackPlugin modules Webpack \`wordpress-require\`
"request": "@wordpress/blob",
"userRequest": "@wordpress/blob",
},
{
"externalType": "import",
"request": "lodash",
"userRequest": "lodash",
},
]
`;

Expand Down Expand Up @@ -363,6 +430,12 @@ exports[`DependencyExtractionWebpackPlugin scripts Webpack \`dynamic-import\` sh
]
`;

exports[`DependencyExtractionWebpackPlugin scripts Webpack \`error\` should produce expected output 1`] = `
"ERROR in main
Module not found: Error: Ensure error in script build.
"
`;

exports[`DependencyExtractionWebpackPlugin scripts Webpack \`function-output-filename\` should produce expected output: Asset file 'chunk--main--main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('lodash', 'wp-blob'), 'version' => 'fc2d750fc9e08c5749db');
"
Expand Down
10 changes: 10 additions & 0 deletions packages/dependency-extraction-webpack-plugin/test/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,16 @@ describe.each( /** @type {const} */ ( [ 'scripts', 'modules' ] ) )(
} )
);

/* eslint-disable jest/no-conditional-expect */
if ( configCase.includes( 'error' ) ) {
expect( stats.hasErrors() ).toBe( true );
expect(
stats.toString( { errors: true, all: false } )
).toMatchSnapshot();
return;
}
/* eslint-enable jest/no-conditional-expect */

if ( stats.hasErrors() ) {
throw new Error(
stats.toString( { errors: true, all: false } )
Expand Down
4 changes: 3 additions & 1 deletion packages/dependency-extraction-webpack-plugin/test/fixtures/combine-assets/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ module.exports = {
new DependencyExtractionWebpackPlugin( {
combineAssets: true,
requestToExternalModule( request ) {
return request.startsWith( '@wordpress/' );
return (
request.startsWith( '@wordpress/' ) || request === 'lodash'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the module tests lodash was not externalized but bundled. Now it is externalized (to prevent errors). This is reflected in the snapshots.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
},
} ),
],
Expand Down
10 changes: 10 additions & 0 deletions packages/dependency-extraction-webpack-plugin/test/fixtures/error/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/* eslint-disable eslint-comments/disable-enable-pair */
/* eslint-disable eslint-comments/no-unlimited-disable */
/* eslint-disable */

import $ from 'jquery';
const apiFetch = await import( '@wordpress/api-fetch' );

$( () => {
apiFetch( { path: '/' } );
} );
15 changes: 15 additions & 0 deletions packages/dependency-extraction-webpack-plugin/test/fixtures/error/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* Internal dependencies
*/
const DependencyExtractionWebpackPlugin = require( '../../..' );

module.exports = {
plugins: [
new DependencyExtractionWebpackPlugin( {
// eslint-disable-next-line no-unused-vars
requestToExternal( request ) {
return new Error( 'Ensure error in script build.' );
},
} ),
],
};
4 changes: 3 additions & 1 deletion ...ndency-extraction-webpack-plugin/test/fixtures/function-output-filename/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ module.exports = {
plugins: [
new DependencyExtractionWebpackPlugin( {
requestToExternalModule( request ) {
return request.startsWith( '@wordpress/' );
return (
request.startsWith( '@wordpress/' ) || request === 'lodash'
);
},
} ),
],
Expand Down
4 changes: 3 additions & 1 deletion ...dependency-extraction-webpack-plugin/test/fixtures/has-extension-suffix/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ module.exports = {
plugins: [
new DependencyExtractionWebpackPlugin( {
requestToExternalModule( request ) {
return request.startsWith( '@wordpress/' );
return (
request.startsWith( '@wordpress/' ) || request === 'lodash'
);
},
} ),
],
Expand Down
4 changes: 3 additions & 1 deletion ...extraction-webpack-plugin/test/fixtures/option-function-output-filename/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ module.exports = {
return `chunk--${ chunkData.chunk.name }--[name].asset.php`;
},
requestToExternalModule( request ) {
return request.startsWith( '@wordpress/' );
return (
request.startsWith( '@wordpress/' ) || request === 'lodash'
);
},
} ),
],
Expand Down
4 changes: 3 additions & 1 deletion ...pendency-extraction-webpack-plugin/test/fixtures/option-output-filename/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ module.exports = {
new DependencyExtractionWebpackPlugin( {
outputFilename: '[name]-foo.asset.php',
requestToExternalModule( request ) {
return request.startsWith( '@wordpress/' );
return (
request.startsWith( '@wordpress/' ) || request === 'lodash'
);
},
} ),
],
Expand Down
Loading
Loading