-
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
Explore ESM modules instead of regular WP scripts #34140
Conversation
/** | ||
* External dependencies | ||
*/ | ||
const esbuild = require( 'esbuild' ); |
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've used esbuild because I didn't manage to use webpack to generate esmodules output, apparently the support is there as experimental but I failed to make it work. Also esbuild is very very very fast.
|
||
esbuild | ||
.build( { | ||
entryPoints: packages.map( ( pkg ) => `./packages/${ pkg }` ), |
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.
For each package, I generate an esm bundle in build/esm/package.js
'moment', | ||
'lodash', | ||
'moment-timezone/moment-timezone', | ||
'moment-timezone/moment-timezone-utils', |
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.
These are "vendors" that are shared across package, so all of these should be modules as well, right now I'm registering them in import-map.php
using skypack esm cdn.
'; | ||
} | ||
|
||
add_action( 'admin_enqueue_scripts', 'gutenberg_inject_import_map' ); |
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.
The import map is injected at the top of all WP pages (admin for now, but could be added to frontend too)
gutenberg_register_import_map_entry( 'react-dom', 'https://cdn.skypack.dev/[email protected]' ); | ||
gutenberg_register_import_map_entry( 'moment', 'https://cdn.skypack.dev/moment/moment' ); | ||
gutenberg_register_import_map_entry( 'moment-timezone/moment-timezone', 'https://cdn.skypack.dev/moment-timezone/moment-timezone' ); | ||
gutenberg_register_import_map_entry( 'moment-timezone/moment-timezone-utils', 'https://cdn.skypack.dev/moment-timezone/moment-timezone-utils' ); |
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 allows us to register the vendors in the import map.
|
||
foreach ( glob( gutenberg_dir_path() . 'build/esm/*.js' ) as $path ) { | ||
$package_name = substr( basename( $path ), 0, -3 ); | ||
gutenberg_register_import_map_entry( '@wordpress/' . $package_name, plugins_url( 'build/esm', __DIR__ ) . '/' . $package_name . '.js' ); |
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 we define the import map entries for all the wp packages. I wasn't able to use the "folder" approach of import maps because the .js
extension is ignored in imports (maybe I should generate extension less files).
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.
What do you mean by "the "folder" approach"?
Also, have you considered not shipping a bundle for every package (#34140 (comment)), but re-exporting those from the combined bundle?
The POC of the idea is at #36716 (comment).
That way we can map a package_name
to an inlined module that re-exports only the given package from the one and only bundle.js. This way we could also re-export packages from 3rd party vendors instead of using CDN.
$tag = str_replace( | ||
sprintf( "<script type=\"module\" src='%s' id='%s-js'></script>", $src, esc_attr( $handle ) ), | ||
sprintf( | ||
'<script id="script-%s" type="module">import %s from "@wordpress/%s"; window.wp = window.wp || {}; window.wp.%s = mod;</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.
backward compatibility for the wp.* global, basically I put in the global the default or all the exports of the module (depends on the module).
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.
Can we use an import map for WP packages, too? I assume, we would have to pass the local path instead and it should work like for vendors. Unless it has some negative impact on the build process.
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.
That's what I'm doing here #34140 (comment)
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 missed that the code calls str_replace
. I understood that both entries are printed 😅
$package_name = substr( $handle, 3 ); | ||
$import = in_array( $package_name, $export_default_packages, true ) ? 'mod' : '* as mod'; | ||
// Replace all scripts with a module script to defer them. | ||
$tag = str_replace( '<script ', '<script type="module" ', $tag ); |
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.
script_loader_tag
is only called once and includes before, after and the script itself:
for the before/after I just mark the script as "type module" to defer it, but for the script source itself, I replace it with the module import.
); | ||
} elseif ( in_array( $handle, $vendors, true ) ) { | ||
$tag = sprintf( | ||
'<script id="script-%s" type="module">import * as mod from "%s"; window.%s = mod;</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 is buggy and might need to be done package per package because the globals for the vendors differ and don't follow any naming guideline.
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.
Could this be retrieved via a custom esbuild/webpack plugin similar to the dependency extraction one?
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.
The number of vendors in WordPress requiring this is pretty small, so we could in theory just hard code these. I don't expect the number of vendor scripts like that to grow much in Core.
$handle, | ||
gutenberg_to_camel_case( $handle ) | ||
); | ||
} elseif ( false === strpos( $handle, 'thickbox' ) && false === strpos( $handle, 'tinymce' ) && false === strpos( $handle, 'clipboard' ) && false === strpos( $handle, 'jquery' ) && false === strpos( $handle, 'hoverintent' ) && false === strpos( $handle, 'moxie' ) ) { |
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.
There are scripts across WP code base that inject <script>
tags without relying on script_loader_tag
filter, meaning if they have dependencies (like tinymce), that dependency shouldn't be a script either, that's why I added all these exceptions. There's probably a better way here.
@@ -1,7 +1,7 @@ | |||
/** | |||
* External dependencies | |||
*/ | |||
import TextareaAutosize from 'react-autosize-textarea'; | |||
//import TextareaAutosize from 'react-autosize-textarea/src'; |
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 package doesn't expose an esm version so I had issues when bundling.
@@ -1,7 +1,7 @@ | |||
/** | |||
* External dependencies | |||
*/ | |||
import { animated } from 'react-spring/web.cjs'; | |||
import { animated } from 'react-spring'; |
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.
Moved to the esm version.
@@ -6,7 +6,7 @@ import classnames from 'classnames'; | |||
|
|||
// react-dates doesn't tree-shake correctly, so we import from the individual | |||
// component here, to avoid including too much of the library | |||
import DayPickerSingleDateController from 'react-dates/lib/components/DayPickerSingleDateController'; | |||
// import DayPickerSingleDateController from 'react-dates/lib/components/DayPickerSingleDateController'; |
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.
react-dates does have an esm version but I wasn't able to use it so I just commented it for now.
import 'moment-timezone/moment-timezone'; | ||
import 'moment-timezone/moment-timezone-utils'; | ||
//import 'moment-timezone/moment-timezone'; | ||
//import 'moment-timezone/moment-timezone-utils'; |
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 didn't manage to rely on the modules for moment-timezone, I had an error that I didn't understand. It will require a deeper look.
The specification (not a W3C Standard) for Import Maps can be found at https://wicg.github.io/import-maps/. It might turn out that we will be using this feature extensively 😄 |
} | ||
|
||
/** | ||
* Hichjack the scripts initialization to use the esmodules instead. |
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.
* Hichjack the scripts initialization to use the esmodules instead. | |
* Hijack the scripts initialization to use the esmodules instead. |
Just wanted to update folks following here: At the moment, I'm thinking the following: Updating core scripts to be esm modules right way is going to be a big breaking change regardless of what we do, so my current thinking is to follow a scaled down but safer approach, like so: 1- Add the APIs to inject insert map and esmodules Now, for the other existing scripts, I'm thinking of exploring this approach: 1- Ship ESM modules for |
I've opened an alternative and smaller PR here to achieve these two steps #34172 |
I just found this |
A playground for the future (#36716).
This is an experiment: The basic idea is that registering and enqueing scripts like WP does was very good for the JS of 10 years ago but today, we want more flexibility, we want modules, lazy-loading... The PR is trying to explore what could a script loader for the next 10 years look like?
To do so it leverages, import maps and es modules. Each WP script becomes a module and WP generates an import map to be able to load these modules synchronously or asynchronously.
The other aspect I tried to test in this PR is the backward compatibility. How can we introduce such system without breaking existing third-party usage and extensibility using the
wp.*
globals. Since<script type="module">
is asynchronous, we can't have a synchronous inline script or external script relying on awp
global generated from a module. So this PR includes a hack that makes all scripts of type "module" diminishing the breaking changes.