Opened 2 months ago
Closed 2 months ago
#65165 closed defect (bug) (fixed)
Script module dependencies may be unavailable on evaluation
| Reported by: | jonsurrell | Owned by: | westonruter |
|---|---|---|---|
| Priority: | high | Milestone: | 7.0 |
| Component: | Script Loader | Version: | 7.0 |
| Severity: | normal | Keywords: | has-patch has-unit-tests needs-dev-note fixed-major dev-reviewed |
| Cc: | Focuses: | javascript |
Description
[61587] / #61500 added module_dependencies to classic scripts, allowing for scripts to depend on registered modules.
Scripts with module dependencies may throw:
TypeError: Failed to resolve module specifier 'example-module'
For example:
<?php wp_register_script_module( 'example-module', '/path/to/example-module.mjs', array(), VERSION ); wp_register_script( 'test-script-module-dependency', false, array(), false, array( 'module_dependencies' => array( 'example-module' ), // ‼️ Enabling one or both of these fixes the issue // 'in_footer' => true, // 'strategy' => 'defer', ), ); wp_enqueue_script( 'test-script-module-dependency' ); wp_add_inline_script( 'test-script-module-dependency', <<<'JAVASCRIPT' (async () => { const m = await import('example-module'); console.log(m); })(); JAVASCRIPT );
This eagerly evaluated script will attempt to resolve the 'example-module' module specifier before the importmap has been printed and will throw the above error. The HTML will be printed like this:
<head> <!-- … --> <script id="test-script-module-dependency-js-after">/*…*/</script> </head> <body> <!-- … --> <script type="importmap">{"imports":{"…":"…"}}</script> </body>
Note that the exact ordering depends on several conditions, such as whether the request is in the WP Admin area, or on the frontend whether or not a block theme is used. However, in my testing the importmap is consistently being printed after the dependent script.
Note the comment in the example. It seems that if the dependent script is deferred or printed in the footer (or ideally, both) then the problem seems to be resolved because the importmap will have been printed already.
Scripts dependencies on modules are a new feature planned for 7.0 added in [61587].
Change History (23)
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
2 months ago
This ticket was mentioned in Slack in #core by jorbin. View the logs.
2 months ago
#4
follow-up:
↓ 5
@
2 months ago
- Priority normal → high
How much effort would be needed to update [61611] if the feature is reverted? If it's low/medium effort, then option A seems like the best option.
Otherwise, I would like to also propose an Option E: Throw a doing_it_wrong if module_dependencies is set without in_footer as true or strategy set as defer.
#5
in reply to: ↑ 4
@
2 months ago
Replying to jorbin:
How much effort would be needed to update [61611] if the feature is reverted? If it's low/medium effort, then option A seems like the best option.
I think espree could be bundled into the javascript-lint.js instead of using a dynamic import. @westonruter I'd love your thought on this.
That should resolve any downsides and seems straightforward.
This ties in with some thoughts I have about private modules. I'd like to be careful and judicious about what modules Core exposes and espree may not make a lot of sense as a Core module.
#6
@
2 months ago
There's a related issue described in this Gutenberg issue that may be addressed by including the polyfill for Firefox.
#7
follow-up:
↓ 9
@
2 months ago
It is sounding more and more like the polyfill is going to be the best option.
This ties in with some thoughts I have about private modules. I'd like to be careful and judicious about what modules Core exposes and espree may not make a lot of sense as a Core module.
@jonsurrell Could you go into a little more details about what you are thinking here? What are the risks associated with espree being available outside of core?
#8
@
2 months ago
- Owner set to
- Status new → accepted
I've followed up on the Gutenberg issue and aforementioned on the AI issue, but I haven't yet dug into this ticket specifically. Neither of those seem to be directly related to the module_dependencies arg for registering scripts.
My plan is to continue following up on this tomorrow.
#9
in reply to: ↑ 7
@
2 months ago
Replying to jorbin:
This ties in with some thoughts I have about private modules. I'd like to be careful and judicious about what modules Core exposes and espree may not make a lot of sense as a Core module.
Could you go into a little more details about what you are thinking here? What are the risks associated with espree being available outside of core?
The general idea is that any modules exposed by Core become part of the WordPress public API. Then it's much more difficult to modify them in any way due to backwards compatibility concerns.
To use espree in a hypothetical example, the latest version is 11.2.0. If version 12.0.0 comes out soon and is full of nice improvements and breaking changes, then Core has a difficult decision to make if it wants to upgrade. Assuming espree@11.2.0 has become part of Core's public API, can Core upgrade without breaking a backwards compatibility? Does it need to add espree12 as another Core module and leave espree as the v11 module indefinitely?
I don't think WordPress Core actually has any business providing an espree module for extenders. Exposing it as a Core module is really an undesired effect of the current limitations of WordPress' module system.
I'd like to expose some modules as "Core-only" by scoping them to be available only to Core. can be achieved technically by using importmap scopes:
<script type="importmap"> { "imports": { "…no espree here…": "…" }, "scopes": { "/wp-includes/js/": { "espree": "/path/to/espree.js?v=123" } } } </script>
This makes it clear that espree is private, it's only exposed to JavaScript under the /wp-includes/js/ path. Extenders cannot access it. espree is clearly private and there are no longer backwards compatibility concers. Core is free to update, change, or remove it at any time in the future.
A more interesting case than espree is when considering syntax highlighting for code editors. It's a similar situation where I don't think Core has any desire to expose a code editor library to extenders, but does have reason to use one internally. A code editor library is more likely to bundle functionality and it would be good to have flexibility to change the exposed functionality or split up the bundle in different ways to optimize its size or loading characteristics.
There are a number of tricky questions to work out, like scoped modules that may be served from a CDN or how a private modules API could be exposed to extenders. For most extenders, there's likely littler reason to use private modules and the global imports: {} is fine. However, I suspect framework-like plugins would likely find private modules useful to differentiate their public modules from internal, private ones.
This is worth a ticket of its own, but I'm glad to share these ideas 🙂
#10
@
2 months ago
Replying to jorbin:
Otherwise, I would like to also propose an Option E: Throw a
doing_it_wrongifmodule_dependenciesis set withoutin_footerastrueorstrategyset asdefer.
I like this suggestion, along with documentation that module_dependencies requires footer printing or defer loading strategy. Eventually, when browsers all support multiple import maps and we just-in-time print importmap scripts before printing scripts and script modules, we can remove this warning. It's true that a script may work around this by waiting to import() until DOMContentLoaded. But we can't know that, so better to warn. We won't block it, so the script may still work correctly, and the doing_it_wrong_trigger_error filter can be used to suppress the warning. However, this won't be needed if we do:
- Add the polyfill and print multiple importmaps as needed.
This would nicely eliminate the timing issues for ensuring that the importmap script is printed after all modules.
I don't think WordPress Core actually has any business providing an espree module for extenders. Exposing it as a Core module is really an undesired effect of the current limitations of WordPress' module system.
This is true, but core has many libraries it makes available in default scripts which extenders can use. So adding espree is no different from previous such libraries (not to say we should keep doing this forever).
In regard to scoped modules, we wouldn't actually have to register espree as a module to begin with. Inside of wp_enqueue_code_editor() we could include the full URL to the espree.js file among the data that gets exported from PHP to JS via the wp.codeEditor global. Then the relevant JS could do await import( wp.codeMirror.path.to.espreeUrl ) as opposed to await import( 'espree' ).
For 7.0, I think the least impactful to go with the documentation and _doing_it_wrong() to warn if module_dependencies is being used without in_footer or defer. For performance, we really want classic scripts to be using these anyway as a best practice, since it eliminates a blocking script from the critical rendering path. Then in 7.1 we can explore adding the printing of multiple importmap scripts along with the polyfill and remove the restriction.
This ticket was mentioned in PR #11788 on WordPress/wordpress-develop by @khokansardar.
2 months ago
#11
- Keywords has-patch has-unit-tests added
Scripts registered or enqueued with a module_dependencies arg may evaluate before the script modules import map is printed if they are loaded blocking in the document head, causing a "Failed to resolve module specifier" error on dynamic imports.
Trigger _doing_it_wrong() from _wp_scripts_add_args_data() when a classic script provides module_dependencies without setting in_footer to true or using a defer loading strategy, and document this requirement in the wp_register_script() and wp_enqueue_script() docblocks.
Existing tests in wpScriptModules.php that exercised this path are updated to use in_footer => true so they continue to validate the import map behavior without tripping the new warning.
@westonruter commented on PR #11788:
2 months ago
#12
Claude picked up on something:
src/wp-includes/script-loader.php:1202-1203—wp-codemirroris registered with no1for the group arg and nodeferstrategy, then hasmodule_dependencies => ['espree']added directly. This matches exactly the antipattern the PR warns against, yet the warning won't fire because it bypasses_wp_scripts_add_args_data(). Either:
wp-codemirroris buggy in core today and should be moved to the footer or givendefer(the import is presumably lazy enough that no one's noticed); or- the warning should be enforced regardless of registration path.
// src/wp-includes/script-loader.php:1202 $scripts->add( 'wp-codemirror', '/wp-includes/js/codemirror/codemirror.min.js', array(), '5.65.20' ); did_action( 'init' ) && $scripts->add_data( 'wp-codemirror', 'module_dependencies', array( 'espree' ) );
Worth either fixing
wp-codemirrorin this PR, or filing a follow-up. Probably also worth duplicating the check intoWP_Scripts::add_data()(alongside the existingmodule_dependenciesvalidation at line 936) so plugins/themes can't slip past the warning by usingwp_scripts()->add_data()directly. The timing concern (group/strategy may not be set yet at the timemodule_dependenciesis added) is real but minor — the check could read the current state and warn if it doesn't look right, accepting some false negatives ifgroupis set later.
Indeed, when I go to /wp-admin/theme-editor.php I see in HEAD:
<script id="wp-codemirror-js" src="http://localhost:8000/wp-includes/js/codemirror/codemirror.min.js?ver=5.65.20"></script> <script id="underscore-js" src="http://localhost:8000/wp-includes/js/underscore.js?ver=1.13.8"></script> <script id="code-editor-js" src="http://localhost:8000/wp-admin/js/code-editor.js?ver=7.1-alpha-62161-src"></script>
And at the end of BODY:
<script id="wp-importmap" type="importmap"> {"imports":{"espree":"http://localhost:8000/wp-includes/js/codemirror/espree.min.js?ver=9.6.1","@wordpress/abilities":"http://localhost:8000/wp-content/plugins/gutenberg/build/modules/abilities/index.js?ver=f3475bc77a30dcc5b38d"}} </script>
In practice, this is not going to be an issue because wp.codeEditor.initialize() is called at DOMContentLoaded:
I asked Claude to look for any instances of wp.codeEditor.initialize() that are called _before_ DONContentLoaded:
They're all triggered after DOMContentLoaded:
| Callsite | Trigger |
| ------------------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|src/js/_enqueues/wp/theme-plugin-editor.js:417|wp.themePluginEditor.init()is invoked via inline scriptjQuery( function( $ ) { wp.themePluginEditor.init( … ); } )(jQuery ready = DOMContentLoaded), theninitCodeEditor()is wrapped in_.defer()on top of that. |
|src/js/_enqueues/wp/customize/controls.js:5456|initSyntaxHighlightingEditor()is called from Customizer control init, which bootstraps after DOMContentLoaded. |
|src/js/_enqueues/wp/widgets/custom-html.js:227| Inside a Backbone view'sinitialize(); widget controls are instantiated only after the Widgets screen bootstraps post-DOMContentLoaded. |
None call
wp.codeEditor.initialize()synchronously during script parse time.
And in admin (the only place
wp-codemirrorships),WP_Script_Modules::print_import_map()is hooked onadmin_print_footer_scriptsat priority 9 — so the import map is in the footer, not the head:
src/wp-includes/class-wp-script-modules.php:450 add_action( 'admin_print_footer_scripts', array( $this, 'print_import_map' ), 9 );By the time
wp.codeEditor.initialize()runs (after DOMContentLoaded), the footer has been parsed and the import map is in the DOM. Soimport('espree')from insideinitialize()resolves correctly today, despitewp-codemirroritself being a head script.
## What this means for the review
wp-codemirroris not buggy — it's a false positive of the heuristic. The heuristic assumes any classic script withmodule_dependencieseither dynamic-imports at parse time or has no other ordering guarantee, which is conservative but reasonable.- The coverage gap I flagged is therefore lower priority. Extending the warning into
WP_Scripts::add_data()would surface a false positive onwp-codemirror, which would force either suppressing the warning there or marking the scriptdeferpurely to silence it.- Recommendation: leave the warning where it is (in
_wp_scripts_add_args_data()only), accept the coverage gap for directadd_data()callers, and optionally adddeferstrategy towp-codemirroras belt-and-suspenders — but it's not required. A code comment nearscript-loader.php:1203explaining "import is deferred viawp.codeEditor.initialize()callers" would prevent future confusion.So the PR is effectively complete as-is. The one substantive issue I raised in the re-review (wp-codemirror coverage gap) turned out to be a false alarm once you trace the actual import timing.
But it feels wrong to not “practice what we preach” here.
#13
@
2 months ago
The latest state of the PR is:
- A warning is issued when using
module_dependencieswithoutin_footerorstrategy=defer. - The
module_dependenciesarg is removed from thecodemirrorentirely in favor of passing the full module URL via the “jshint” config. This is becausecodemirroris printed in thehead, which violates the proposed constraint.
#14
@
2 months ago
For 7.0, I agree with the _doing_it_wrong() approach. Its impact is minimal and should help to address the root of the problem. The downside is that it's possible to use module dependencies correctly without loading in the footer, to the warnings may produce noise in valid cases. At this point in the release cycle that tradeoff seems acceptable.
For a future release, I'd like to see an improved solution that leverages multiple importmaps that can be printed as necessary.
#15
follow-up:
↓ 17
@
2 months ago
- Keywords needs-dev-note added
For a future release, I'd like to see an improved solution that leverages multiple importmaps that can be printed as necessary.
100% agree. This might be something we just get when Firefox rolls out it's support more broadly so we should follow things there and potentially just wait, but I think it's worth ticketing for 7.1 so a proper decision can be made.
I am adding needs-devnote since I think this would be good to document. I see that module_dependencies don't have a devnote, so if it is getting one, I am very ok with this being a part of that.
@westonruter commented on PR #11788:
2 months ago
#16
@sirreal What do you think of 3866c55? While all uses of core call wp.codeEditor.initialize() at or after DCL, it's possible that a plugin may not be.
Easy way to test this is to apply this plugin code which adds an editor in the admin notice area (yay!):
add_action( 'admin_notices', static function () { wp_scripts()->do_items( array( 'code-editor' ) ); wp_styles()->do_items( array( 'code-editor' ) ); ?> <textarea id="admin-notice-code-editor"></textarea> <?php wp_print_inline_script_tag( sprintf( 'wp.codeEditor.initialize( document.getElementById( "admin-notice-code-editor" ), %s );', wp_json_encode( wp_get_code_editor_settings( array( 'type' => 'text/javascript' ) ), JSON_HEX_TAG | JSON_UNESCAPED_SLASHES ) ) ); } );
Then when going to the Dashboard in Firefox, you can see in the console:
⚠️ wp.codeEditor.initialize() should be invoked at or after DOMContentLoaded. This is to ensure the importmap has been parsed prior to performing the dynamic import of espree in javascript-lint.js. code-editor.js:418:12
⚠️ Import maps are not allowed after a module load or preload has started. index.php
❌ Uncaught TypeError: The specifier “@wordpress/abilities” was a bare specifier, but was not remapped to anything. Relative module specifiers must start with “./”, “../” or “/”. index.js:44:58
❌ Uncaught TypeError: The specifier “@wordpress/abilities” was a bare specifier, but was not remapped to anything. Relative module specifiers must start with “./”, “../” or “/”. index.php:1877:31
❌ Uncaught (in promise) TypeError: The specifier “@wordpress/boot” was a bare specifier, but was not remapped to anything. Relative module specifiers must start with “./”, “../” or “/”. index.php
So that first warning gives a clue about the subsequent ones.
Note that in Chrome, only this new warning from wp.codeEditor.initialize() is emitted. There are no other warnings or errors.
#17
in reply to: ↑ 15
@
2 months ago
Replying to jorbin:
I am adding
needs-devnotesince I think this would be good to document. I see that module_dependencies don't have a devnote, so if it is getting one, I am very ok with this being a part of that.
The keyword is also on #61500 which introduced module_dependencies. I am still planning to write a dev note for that, but I was first working on the dev note for the CodeMirror upgrade, which I'm also behind on!
@jonsurrell commented on PR #11788:
2 months ago
#18
What do you think of 3866c55?
I've tried some other things and what you've here seems like the best tradeoff 👍
---
I marked the code-editor script with in_footer and defer, but that seems error prone and highly dependent on how the script is used by extenders. For example, initializing a code editor with wp_add_inline_script( 'code-editor', /*…*/ ) seems to respect in_footer but not defer, leading to unpredictable results.
I also considered rescheduling the editor initialization, but because the initialize function returns a code editor instance this is also potentially breaking.
<details><summary>rescheduling diff</summary>
-
src/js/_enqueues/wp/code-editor.js
diff --git i/src/js/_enqueues/wp/code-editor.js w/src/js/_enqueues/wp/code-editor.js index 7145f4c7ba..8a47e0256f 100644
i w if ( 'undefined' === typeof window.wp.codeEditor ) { 415 415 */ 416 416 wp.codeEditor.initialize = function initialize( textarea, settings ) { 417 417 if ( document.readyState === 'loading' ) { 418 console.warn( 'wp.codeEditor.initialize() should be invoked at or after DOMContentLoaded. This is to ensure the importmap has been parsed prior to performing the dynamic import of espree in javascript-lint.js.' ); 418 console.warn( 419 'wp.codeEditor.initialize() must be invoked at or after DOMContentLoaded. This initialization has been deferred.' 420 ); 421 document.addEventListener( 422 'DOMContentLoaded', 423 () => { 424 wp.codeEditor.initialize( textarea, settings ); 425 }, 426 { once: true } 427 ); 428 return; 419 429 } 420 430 421 431 let $textarea;
</details>
#20
@
2 months ago
- Keywords dev-feedback fixed-major added
- Resolution fixed
- Status closed → reopened
Re-opening for backporting r62368 to the 7.0 branch.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
There is a difficult balance we've already observed with script modules and blocks where it's important to print the importmap late enough that all module dependencies have already been discovered, but early enough that it can be used by scripts and modules that require it.
All major browsers now support multiple import maps with the notable exception of Firefox. Firefox 150 (the current version) has added support for multiple import maps, but it's behind a configuration flag that defaults to disabled.
Using multiple import maps is ideal, allowing for multiple importmaps to be printed as necessary. There is a robust polyfill that was already added ([57492]) and later removed ([58952]) due to good browser support for basic importmap functionality. It may be worth adding the polyfil for the multiple importmap support.
Some possible paths forward in no particular order:
DOMContentLoadedbefore attempting to import a module. (This seems undesirable for a number of reasons.)Options
aorbseem preferable. Removing the feature from 7.0 is safe.bwill require more development late in the 7.0 cycle to address this issue.I'd welcome other folks ideas and opinions here.
This was discovered here with help from @dkotter and @gziolo.
FYI @westonruter.