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.

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

Script Modules API: Backport - Add import map polyfill #5947

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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: 7 additions & 5 deletions Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -1559,16 +1559,18 @@ module.exports = function(grunt) {
} );

/**
* Build assertions for the lack of source maps in JavaScript files.
* Compiled JavaScript files may link to sourcemaps. In some cases,
* the source map may not be available, which can cause 404 errors when
* browsers try to download the sourcemap from the referenced URLs.
* Ensure that sourcemap links are not included in JavaScript files.
*
* @ticket 24994
* @ticket 46218
* @ticket 60348
*/
grunt.registerTask( 'verify:source-maps', function() {
gziolo marked this conversation as resolved.
Show resolved Hide resolved
const ignoredFiles = [
'build/wp-includes/js/dist/components.js',
'build/wp-includes/js/dist/block-editor.js',
'build/wp-includes/js/dist/block-editor.min.js'
];
const files = buildFiles.reduce( ( acc, path ) => {
// Skip excluded paths and any path that isn't a file.
Expand All @@ -1591,10 +1593,10 @@ module.exports = function(grunt) {
encoding: 'utf8',
} );
// `data:` URLs are allowed:
const match = contents.match( /sourceMappingURL=((?!data:).)/ );
const doesNotHaveSourceMap = ! /^\/\/# sourceMappingURL=((?!data:).)/m.test(contents);
Copy link
Member

@sirreal sirreal Jan 30, 2024

Choose a reason for hiding this comment

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

Sharing notes on this change that I co-authored with @c4rl0sbr4v0.

First, a minor change. We use RegExp.test() instead of String.match(). We don't care about the result of matching, we want to know "does this match," so RegExp.test() (with its boolean result) is ideal.

Second, and more important. The expression was not specific enough and was matching JavaScript source files that were, themselves, looking for sourcemap comments. In this PR is was matching this line from es-module-shims:

  const sourceMapURLCommentPrefix = '\n//# sourceMappingURL=';

We change to better match a source map comment by anchoring the expression to the start of the line and matching the comment opening //# . This fixes the false positives and allows us to remove several files from the ignore list.


assert(
match === null,
doesNotHaveSourceMap,
`The ${ file } file must not contain a sourceMappingURL.`
);
} );
Expand Down
11 changes: 11 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@
"clipboard": "2.0.11",
"core-js-url-browser": "3.6.4",
"element-closest": "^3.0.2",
"es-module-shims": "1.8.2",
"formdata-polyfill": "4.0.10",
"framer-motion": "10.16.4",
"hoverintent": "2.2.1",
Expand Down
15 changes: 15 additions & 0 deletions src/wp-includes/class-wp-script-modules.php
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,26 @@ public function print_script_module_preloads() {
/**
* Prints the import map using a script tag with a type="importmap" attribute.
*
* @global WP_Scripts $wp_scripts The WP_Scripts object for printing the polyfill.
* @since 6.5.0
*/
public function print_import_map() {
$import_map = $this->get_import_map();
if ( ! empty( $import_map['imports'] ) ) {
global $wp_scripts;
if ( isset( $wp_scripts ) ) {
Copy link
Member

@gziolo gziolo Jan 31, 2024

Choose a reason for hiding this comment

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

What would be the case when $wp_scripts is not set? I'm asking because I don't see it guarded anywhere else in the codebase?

I see some ! empty( $wp_scripts ) checks.

wp_print_inline_script_tag(
wp_get_script_polyfill(
$wp_scripts,
array(
'HTMLScriptElement.supports && HTMLScriptElement.supports("importmap")' => 'wp-polyfill-importmap',
)
),
array(
'id' => 'wp-load-polyfill-importmap',
)
);
}
wp_print_inline_script_tag(
wp_json_encode( $import_map, JSON_HEX_TAG | JSON_HEX_AMP ),
array(
Expand Down
2 changes: 2 additions & 0 deletions src/wp-includes/script-loader.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ function wp_default_packages_vendor( $scripts ) {
'lodash',
'wp-polyfill-fetch',
'wp-polyfill-formdata',
'wp-polyfill-importmap',
'wp-polyfill-node-contains',
'wp-polyfill-url',
'wp-polyfill-dom-rect',
Expand All @@ -120,6 +121,7 @@ function wp_default_packages_vendor( $scripts ) {
'wp-polyfill-object-fit' => '2.3.5',
'wp-polyfill-inert' => '3.1.2',
'wp-polyfill' => '3.15.0',
'wp-polyfill-importmap' => '1.8.2',
);

foreach ( $vendor_scripts as $handle => $dependencies ) {
Expand Down
30 changes: 30 additions & 0 deletions tests/phpunit/tests/dependencies/scripts.php
Original file line number Diff line number Diff line change
Expand Up @@ -3104,6 +3104,36 @@ public function test_wp_scripts_move_to_footer( $set_up, $expected_header, $expe
$this->assertEquals( $expected_groups, wp_scripts()->groups, 'Expected groups to match.' );
}

/**
* Test that get_script_polyfill() returns the correct polyfill.
*
* @ticket 60348
*
* @covers ::wp_get_script_polyfill
*
* @global WP_Scripts $wp_scripts WP_Scripts instance.
* @param string $script_name Script name.
* @param string $test_script Conditional that checks browser compatibility.
* @param string $script_url Script source URL.
*/
public function test_wp_get_script_polyfill() {
global $wp_scripts;
$script_name = 'wp-polyfill-importmap';
$test_script = 'HTMLScriptElement.supports && HTMLScriptElement.supports("importmap")';
$script_url = 'https://example.com/wp-polyfill-importmap.js';
wp_register_script( $script_name, $script_url, array(), null, true );

$polyfill = wp_get_script_polyfill(
$wp_scripts,
array(
$test_script => $script_name,
)
);
$expected = '( ' . $test_script . ' ) || document.write( \'<script src="' . $script_url . '"></scr\' + \'ipt>\' );';

$this->assertEquals( $expected, $polyfill );
}

/**
* Data provider for test_wp_scripts_move_to_footer.
*
Expand Down
33 changes: 32 additions & 1 deletion tests/phpunit/tests/script-modules/wpScriptModules.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
*
* @coversDefaultClass WP_Script_Modules
*/
class Tests_WP_Script_Modules extends WP_UnitTestCase {
class Tests_Script_Modules_WpScriptModules extends WP_UnitTestCase {

/**
* Instance of WP_Script_Modules.
*
Expand All @@ -24,6 +25,7 @@ class Tests_WP_Script_Modules extends WP_UnitTestCase {
*/
public function set_up() {
parent::set_up();
// Set up the WP_Script_Modules instance.
$this->script_modules = new WP_Script_Modules();
}

Expand Down Expand Up @@ -600,4 +602,33 @@ public function test_wp_enqueue_script_module_registers_all_params() {
$this->assertCount( 1, $import_map );
$this->assertStringStartsWith( '/dep.js', $import_map['dep'] );
}

/**
* Tests that the import map polyfill is not enqueued when there are no
* modules registered.
*
* @ticket 60348
*
* @covers ::print_import_map_polyfill()
*/

public function test_wp_print_import_map_has_polyfill() {
// Polyfill is empty when no modules are registered.
$import_map_polyfill = get_echo( array( $this->script_modules, 'print_import_map' ) );

$this->assertEquals( '', $import_map_polyfill );

// Polyfill is not empty when a module is registered.
wp_register_script( 'wp-polyfill-importmap', '/wp-polyfill-importmap.js', array(), '1.0' );

$this->script_modules->enqueue( 'foo', '/foo.js', array( 'dep' ), '1.0' );
$this->script_modules->register( 'dep', '/dep.js' );
$import_map_polyfill = get_echo( array( $this->script_modules, 'print_import_map' ) );
$p = new WP_HTML_Tag_Processor( $import_map_polyfill );
$p->next_tag( array( 'tag' => 'SCRIPT' ) );
$id = $p->get_attribute( 'id' );

$this->assertEquals( 'wp-load-polyfill-importmap', $id );
wp_deregister_script( 'wp-polyfill-importmap' );
}
}
2 changes: 2 additions & 0 deletions tools/webpack/packages.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ module.exports = function (
'wp-polyfill-object-fit.js':
'objectFitPolyfill/src/objectFitPolyfill.js',
'wp-polyfill-inert.js': 'wicg-inert/dist/inert.js',
'wp-polyfill-importmap.js': 'es-module-shims/dist/es-module-shims.wasm.js',
'moment.js': 'moment/moment.js',
'react.js': 'react/umd/react.development.js',
'react-dom.js': 'react-dom/umd/react-dom.development.js',
Expand Down Expand Up @@ -121,6 +122,7 @@ module.exports = function (
'polyfill-library/polyfills/__dist/Node.prototype.contains/raw.js',
'wp-polyfill-dom-rect.min.js':
'polyfill-library/polyfills/__dist/DOMRect/raw.js',
'wp-polyfill-importmap.min.js': 'es-module-shims/dist/es-module-shims.wasm.js',
};

const phpFiles = {
Expand Down
Loading