WordPress.org
Get WordPress

Make WordPress Core

Opened 5 days ago

Last modified 27 hours ago

#65165 accepted defect (bug)

Script module dependencies may be unavailable on evaluation

Reported by: jonsurrell's profile jonsurrell Owned by: westonruter's profile westonruter
Milestone: 7.0 Priority: high
Severity: normal Version: trunk
Component: Script Loader Keywords:
Focuses: javascript Cc:

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 (10)

#1 @jonsurrell
5 days 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 days ago by jonsurrell (previous) (diff)

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

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

#4 follow-up: @jorbin
3 days ago

  • Priority changed from normal to 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
3 days 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 39 hours ago by jonsurrell (previous) (diff)

#6 @jonsurrell
2 days 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
2 days 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
42 hours ago

  • Owner set to westonruter
  • Status changed from new to 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
40 hours 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
27 hours 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.

Note: See TracTickets for help on using tickets.