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 #5818

Closed
wants to merge 36 commits into from
Closed

Conversation

luisherranz
Copy link
Member

@luisherranz luisherranz commented Dec 22, 2023

Trac ticket: https://core.trac.wordpress.org/ticket/56313


EDIT: I've updated the description to showcase the latest changes.

This PR proposes a new Modules API for WordPress, designed to work with native ES Modules and Import Maps. It introduces functions such as wp_register_script_module, and wp_enqueue_script_module.

The API aims to provide a familiar experience to the existing WP_Scripts class, offering similar functionality. However, it's not intended to duplicate the exact functionality of WP_Scripts; rather, it is carefully tailored to address the specific needs and capabilities of ES modules.

For this initial version, the current proposal is intentionally simplistic, covering only the essential features needed to work with ES modules. Other enhancements and optimizations can be added later as the community identifies additional requirements and use cases.

Differences Between WP_Script_Modules and WP_Scripts

  1. Dependency Specification:

    With WP_Script_Modules, the array of dependencies supports not just strings but also arrays that include the dependency import type (static or dynamic). This design choice allows for future extensions of dependency properties, such as adding a version property to support "scopes" within import maps.

  2. Module Identifier:

    Instead of a handle, WP_Script_Modules utilizes the module identifier, aligning with the module identifiers used in JavaScript files and import maps.

  3. Deregistration:

    There is no equivalent of wp_deregister_module at this stage.

API

wp_register_script_module( $module_identifier, $src, $deps, $version )

Registers a module.

// Registers a module with dependencies and versioning.
wp_register_script_module(
  'my-module',
  '/path/to/my-module.js',
  array( 'static-dependency-1', 'static-dependency-2' ),
  '1.2.3'
);
// my-module.js
import { ... } from 'static-dependency-1';
import { ... } from 'static-dependency-2';

// ...
// Registers a module with a dynamic dependency.
wp_register_script_module(
  'my-module',
  '/path/to/my-module.js',
  array(
    'static-dependency',
    array(
      'id'   => 'dynamic-dependency',
      'import' => 'dynamic'
    ),
  )
);
// my-module.js
import { ... } from 'static-dependency';

// ...
const dynamicModule = await import('dynamic-dependency');

wp_enqueue_script_module( $module_identifier, $src, $deps, $version )

Enqueues a module. If it includes a src, it will also register the module.

wp_enqueue_script_module( 'my-module' );

wp_dequeue_script_module( $module_identifier )

Dequeues a module.

wp_dequeue_script_module( 'my-module' );

Output

  • When modules are enqueued, they are printed within script tags containing type="module" attributes.
  • Additionally, static dependencies of enqueued modules utilize link tags with rel="modulepreload" attributes.
  • Lastly, an import map is generated and inserted using a <script type="importmap"> tag.
<script type="module" src="/path/to/my-module.js" id="my-module"></script>
<link rel="modulepreload" href="/path/to/static-dependency.js" id="static-dependency" />
<script type="importmap">
  {
    "imports": {
      "static-dependency": "/path/to/static-dependency.js",
      "dynamic-dependency": "/path/to/dynamic-dependency.js"
    }
  }
</script>

Import Map Polyfill Requirement

Even though all the major browsers already support import maps, an import map polyfill is required until the percentage of users using old browser versions without import map support drops significantly.

This work is ongoing and will be added to this PR once it's ready. Contributors can follow the progress in the associated GitHub issue: guybedford/es-module-shims#406.

Feedback

This Modules API proposal is open for feedback and further discussion. Please contribute ideas, potential use cases, or any other thoughts that could make this feature robust and broadly useful for the WordPress community. Please use the Trac Ticket or this pull request.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@luisherranz
Copy link
Member Author

luisherranz commented Dec 22, 2023

I see that PHP 8.3 has some deprecations when using ReflectionProperty. I'll review and fix those.

EDIT: I switched to setStaticPropertyValue/getStaticPropertyValue but now PHP 7.0 is failing. I'll investigate more 😄

EDIT 2: Fixed now in f13a068!

@luisherranz
Copy link
Member Author

Thanks for the review, @swissspidy. I've made all the necessary changes 🙂

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I used this PR to build out what it would take to handle viewModule in block.json (analagous module version of viewScript).

One thing that I encountered is that module registration returns null. We don't have a return value that indicates success or failure. Look at this block:

$result = wp_register_script(
$script_handle,
$script_uri,
$script_dependencies,
isset( $script_asset['version'] ) ? $script_asset['version'] : false,
$script_args
);
if ( ! $result ) {
return false;
}

When implementing similar handling for modules, there's no return value from wp_register_module or WP_Modules::register, so we end up optimistically continuing as if the module is correctly registered.

Was that an intentional design decision? I'd expect to have some return value, maybe the module ID or an array that's descriptive of the module ([ id => …, src => …, …]) or false in case of failure.

@luisherranz
Copy link
Member Author

When I was studying WP Scripts, I saw that wp_register_script returns false when the script is already registered and true otherwise:

if ( isset( $this->registered[ $handle ] ) ) {
  return false;
}
$this->registered[ $handle ] = new _WP_Dependency( $handle, $src, $deps, $ver, $args );
// ...
return true;

code

So, it works like this:

  • Returns true: the script has been successfully registered.
  • Returns false: the script hasn't been registered again, but it was successfully registered before, so it is successfully registered.

In both cases, the script is successfully registered.

so we end up optimistically continuing as if the module is correctly registered

That's my point. It doesn't matter what WP Scripts returns, the script is always registered so we should be able to continue as if the script is correctly registered.

I'm not opposed to mimicking the true/false return of WP Scripts, but I'd like to understand the use cases first in case there's an opportunity for improvement in the API 🙂

So, is there a use case where that true/false return is actually useful information?

@sirreal
Copy link
Member

sirreal commented Dec 28, 2023

Good point. I agree, it doesn't seem very helpful to distinguish between "registered on this call" and "already registered" if the result is the same. If there's no real failure scenario, it seems fine to always return null.

@luisherranz
Copy link
Member Author

Ok. Thanks, Jon. Let's keep it as it is for now then, just to keep open the possibility of returning something useful in the future or until we discover of use case for the WP Scripts true/false behavior 🙂

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@luisherranz The logic here looks great to me.

I do however think the implementation should rely on a non-static class, as only stateless classes/methods should be static. The existing WP_Scripts and WP_Styles classes aren't static either, so that's another (albeit minor) reason.

src/wp-includes/class-wp-modules.php Outdated Show resolved Hide resolved
src/wp-includes/modules.php Outdated Show resolved Hide resolved
src/wp-includes/default-filters.php Outdated Show resolved Hide resolved
tests/phpunit/tests/modules/modules.php Outdated Show resolved Hide resolved
tests/phpunit/tests/modules/modules.php Outdated Show resolved Hide resolved
tests/phpunit/tests/modules/modules.php Outdated Show resolved Hide resolved
tests/phpunit/tests/modules/modules.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-modules.php Outdated Show resolved Hide resolved
@gziolo
Copy link
Member

gziolo commented Jan 3, 2024

Good point. I agree, it doesn't seem very helpful to distinguish between "registered on this call" and "already registered" if the result is the same. If there's no real failure scenario, it seems fine to always return null.

In the case of WP_Scripts, it bails out early when there is the same handle registered, and therefore prevents overriding the same handle by plugins. That's why wp_deregister_script exists, so it was still possible. Example usage in the Gutenberg plugin:

https://github.com/WordPress/gutenberg/blob/d3071ca03bf3db4cd4267b4d9eb48c5658491c0f/lib/blocks.php#L219-L232

It's going to be challenging to replicate the same code with the currently proposed approach when the only possibility is to dequeue the module. In the shared example, all these scripts might not be enqueued as it happens later in the process. However, maybe it's fine, as when using the proposed implementation for modules, the plugin can register the same handles again, and they will get successfuly overridden. The only difference would be that some unused modules would remain registered and hopefully never enqueued.

WP_Scripts also offers wp_script_is that allows checking whether the script was already registered or enqueued. In that sense, it's similar to what the false return value does when calling wp_register_script that tries to register a previously registered script handle.

@luisherranz
Copy link
Member Author

@felixarntz: thanks! It makes sense. I'll make those changes 🙂

@gziolo: So the use case for wp_deregister_script is to prevent the script from being enqueued in the situation where someone else is still calling wp_enqueue_script?

@gziolo
Copy link
Member

gziolo commented Jan 3, 2024

So the use case for wp_deregister_script is to prevent the script from being enqueued in the situation where someone else is still calling wp_enqueue_script?

Yes, this is how it works. There is a check that ensures that only registered scripts get printed to make it possible to safely deregister a script.

@westonruter
Copy link
Member

@luisherranz Interesting. I wasn't aware of JSON and CSS also being on the roadmap for being modules. Nevertheless, I think WP_Script_Modules is still appropriate for them as well, as you listed they all mention "script":

  • JavaScript modules
  • CSS module scripts
  • JSON (JavaScript Object Notation) modules

Additionally, all of these modules alike end up getting printed in an importmap script, and more importantly, they all get used in a script context. So they are all inherently "scripty".

@luisherranz
Copy link
Member Author

Ok! Thanks, Weston. I've renamed the class to WP_Script_Modules.

I've also added support to register the script with a single call to the enqueue function, as requested by @aaronjorbin in the ticket.

I still agree that it would have made sense to separate both registration and enqueuing as a way to teach people that those are two separate steps (especially when working with modules where registered modules can't only end up being enqueued, but also preloaded or added to the import map), but as @aaronjorbin pointed out, the wp_enqueue_style is still going to be relevant in the future, and it would be confusing to have different APIs for both:

wp_enqueue_style( 'my-style', '/styles.css' );
wp_register_module( 'my-module', '/module.js' );
wp_enqueue_module( 'my-module' );

So now, this also works:

wp_enqueue_style( 'my-style', '/styles.css' );
wp_enqueue_module( 'my-module', '/module.js' );

From my point of view, I think this is in a great state. I don't have plans to add any other changes to this initial version.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM!

@ockham
Copy link
Contributor

ockham commented Jan 11, 2024

Now that the WP_Modules class has been renamed to WP_Script_Modules -- should the wp_modules() function also be renamed to wp_script_modules() (for consistency)?

@luisherranz @westonruter

@luisherranz
Copy link
Member Author

I'd keep it as wp_modules to keep consistency with the register and enqueue functions.

@ockham
Copy link
Contributor

ockham commented Jan 11, 2024

I'd keep it as wp_modules to keep consistency with the register and enqueue functions.

Works for me! And if we end up deciding otherwise, we can always change it in a follow-up 😊

@ockham
Copy link
Contributor

ockham commented Jan 11, 2024

Committed to Core in https://core.trac.wordpress.org/changeset/57269.

Thank you, folks! This is big 🎉

@ockham ockham closed this Jan 11, 2024
@luisherranz
Copy link
Member Author

Follow-up tickets:

@felixarntz
Copy link
Member

felixarntz commented Jan 11, 2024

It's great to see this committed! 🎉

Nevertheless, I think the naming topic was wrapped a bit quickly as it was only initiated 2 days ago by @westonruter, so that didn't leave much room for other people's thoughts before the commit. We can of course follow up if needed, but a final decision on naming needs to happen soon before more other things are implemented on top of the API.

I think we should continue the discussion, either here or in a new issue. I personally find it a confusing choice to go with WP_Script_Modules class but wp_*_module() function names. What's the rationale to do it differently between those two places? To me it doesn't seem to make sense.

  • Either we go with the more verbose script modules, in expectation ("future proof") that other kinds of modules will be made available via WordPress APIs in the future.
  • Or we stick to just "module" and agree on that, when we talk about "modules" in WordPress context, it's always JS modules.

I don't have a strong opinion which way to go, but I think the current solution is neither.

@westonruter
Copy link
Member

westonruter commented Jan 11, 2024

IMO, I think the functions should be wp_script_modules(), wp_register_script_module(), and wp_enqueue_script_module() for consistency.

@gziolo
Copy link
Member

gziolo commented Jan 11, 2024

IMO, I think the functions should be wp_script_modules(), wp_register_script_module(), and wp_enqueue_script_module() for consistency.

It makes perfect sense after the class name changes were applied.

@azaozz
Copy link
Contributor

azaozz commented Jan 12, 2024

I think the functions should be wp_script_modules(), wp_register_script_module(), and wp_enqueue_script_module() for consistency.

Another +1 for this.

@luisherranz
Copy link
Member Author

Seems unanimous, then. I'll prepare a PR with the renaming.

@luisherranz luisherranz changed the title Modules API Script Modules API Jan 15, 2024
@luisherranz
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants