Make WordPress Core

Opened 5 months ago

Closed 5 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)

#1 @jonsurrell
5 months ago

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:

  1. Revert [61587] for 7.0, removing the script->module dependency feature and the bug. [61611] would also need a solution as it depends on the feature.
  2. Add the polyfill and print multiple importmaps as needed.
  3. Always defer scripts that have a dependency on a script module. (This seems heavy handed and likely to introduce bugs.)
  4. Document the situation and do nothing. Scripts can depend on modules, but authors must understand this limitation and can apply workarounds such as deferring their script or waiting for DOMContentLoaded before attempting to import a module. (This seems undesirable for a number of reasons.)

Options a or b seem preferable. Removing the feature from 7.0 is safe. b will 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.

Last edited 5 months ago by jonsurrell (previous) (diff)

This ticket was mentioned in ​Slack in #core-performance by westonruter. ​View the logs.


5 months ago

This ticket was mentioned in ​Slack in #core by jorbin. ​View the logs.


5 months ago

#4 follow-up: @jorbin
5 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 @jonsurrell
5 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.

Last edited 5 months ago by jonsurrell (previous) (diff)

#6 @jonsurrell
5 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: @jorbin
5 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 @westonruter
5 months ago

  • Owner set to westonruter
  • 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 @jonsurrell
5 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 @westonruter
5 months ago

Replying to jorbin:

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.

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:

  1. 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.


5 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.

See #65165, #61500.

​@westonruter commented on ​PR #11788:


5 months ago
#12

Claude picked up on something:

src/wp-includes/script-loader.php:1202-1203 — wp-codemirror is registered with no 1 for the group arg and no defer strategy, then has module_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-codemirror is buggy in core today and should be moved to the footer or given defer (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-codemirror in this PR, or filing a follow-up. Probably also worth duplicating the check into WP_Scripts::add_data() (alongside the existing module_dependencies validation at line 936) so plugins/themes can't slip past the warning by using wp_scripts()->add_data() directly. The timing concern (group/strategy may not be set yet at the time module_dependencies is added) is real but minor — the check could read the current state and warn if it doesn't look right, accepting some false negatives if group is 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:

​https://github.com/WordPress/wordpress-develop/blob/20b5d10910b0d598b99313bc27f16a52cfb8f476/src/wp-admin/plugin-editor.php#L166

​https://github.com/WordPress/wordpress-develop/blob/20b5d10910b0d598b99313bc27f16a52cfb8f476/src/wp-admin/theme-editor.php#L151

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 script jQuery( function( $ ) { wp.themePluginEditor.init( … ); } ) (jQuery ready = DOMContentLoaded), then initCodeEditor() 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's initialize(); 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-codemirror ships), WP_Script_Modules::print_import_map() is hooked on admin_print_footer_scripts at 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. So import('espree') from inside initialize() resolves correctly today, despite wp-codemirror itself being a head script.

## What this means for the review

  • wp-codemirror is not buggy — it's a false positive of the heuristic. The heuristic assumes any classic script with module_dependencies either 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 on wp-codemirror, which would force either suppressing the warning there or marking the script defer purely to silence it.
  • Recommendation: leave the warning where it is (in _wp_scripts_add_args_data() only), accept the coverage gap for direct add_data() callers, and optionally add defer strategy to wp-codemirror as belt-and-suspenders — but it's not required. A code comment near script-loader.php:1203 explaining "import is deferred via wp.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 @westonruter
5 months ago

The latest state of the ​PR is:

  1. A warning is issued when using module_dependencies without in_footer or strategy=defer.
  2. The module_dependencies arg is removed from the codemirror entirely in favor of passing the full module URL via the “jshint” config. This is because codemirror is printed in the head, which violates the proposed constraint.

#14 @jonsurrell
5 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: @jorbin
5 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:


5 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:

https://github.com/user-attachments/assets/11a93d29-d3e6-4bfe-816a-56a89aabfd23

⚠️ 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 @westonruter
5 months ago

Replying to jorbin:

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.

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:


5 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 ) {  
    415415         */
    416416        wp.codeEditor.initialize = function initialize( textarea, settings ) {
    417417                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;
    419429                }
    420430
    421431                let $textarea;

</details>

#19 @westonruter
5 months ago

  • Resolution → fixed
  • Status accepted → closed

In 62368:

Script Loader: Warn when classic scripts with module dependencies lack footer/defer.

A classic script with module_dependencies may be evaluated before the script modules import map is printed if it loads 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.
  • Remove the module_dependencies arg from the wp-codemirror script registration in favor of passing the espree module URL directly through wp_get_code_editor_settings(). This avoids registering espree as a publicly-available script module when it is only ever used internally as a private implementation detail of the code editor.
  • Add a console.warn() in wp.codeEditor.initialize() when invoked before DOMContentLoaded, so developers are alerted if the function is called too early for the import map to have been parsed.
  • Add PHPStan types which were missing when module_dependencies were initially introduced.
  • Harden WP_Scripts::add_data() against non-string strategy values being passed to sprintf().

Developed in ​https://github.com/WordPress/wordpress-develop/pull/11788

Follow-up to r61587.

Props khokansardar, westonruter, jonsurrell, jorbin.
See #61500, #64238.
Fixes #65165.

#20 @westonruter
5 months ago

  • Keywords dev-feedback fixed-major added
  • Resolution fixed
  • Status closed → reopened

Re-opening for backporting r62368 to the 7.0 branch.

This ticket was mentioned in ​Slack in #core by audrasjb. ​View the logs.


5 months ago

#22 @jonsurrell
5 months ago

  • Keywords dev-reviewed added; dev-feedback removed

[62368] Looks good for 7.0 backport.

#23 @ellatrix
5 months ago

  • Resolution → fixed
  • Status reopened → closed

In 62379:

Script Loader: Warn when classic scripts with module dependencies lack footer/defer.

A classic script with module_dependencies may be evaluated before the script modules import map is printed if it loads 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.
  • Remove the module_dependencies arg from the wp-codemirror script registration in favor of passing the espree module URL directly through wp_get_code_editor_settings(). This avoids registering espree as a publicly-available script module when it is only ever used internally as a private implementation detail of the code editor.
  • Add a console.warn() in wp.codeEditor.initialize() when invoked before DOMContentLoaded, so developers are alerted if the function is called too early for the import map to have been parsed.
  • Add PHPStan types which were missing when module_dependencies were initially introduced.
  • Harden WP_Scripts::add_data() against non-string strategy values being passed to sprintf().

Developed in ​https://github.com/WordPress/wordpress-develop/pull/11788

Follow-up to r61587.

Reviewed by jonsurrell.
Merges [62368] to the 7.0 branch.
Props khokansardar, westonruter, jonsurrell, jorbin.
See #61500, #64238.
Fixes #65165.

Note: See TracTickets for help on using tickets.