#56313 closed enhancement (fixed)
Add the ability to handle ES Modules and Import Maps
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Script Loader | Keywords: | has-patch has-unit-tests needs-dev-note |
Focuses: | javascript, performance | Cc: |
Description
wp_enqueue_script should be augmented to add scripts with type="module" to pages.
Or a new function like wp_enqueue_module etc. should be created.
Also having the ability to ad a nomodule scrip-tag should be considered, even though ES6 ans ES2020 module support is very wide spread.
Especially ES2020 dynamic modules are a feature that really should be available for theme developers, as it is the best (standard conform) way to handle dynamically loading JS at runtime.
Change History (85)
#1
@
3 years ago
- Focuses javascript added
- Keywords 2nd-opinion added
- Severity changed from major to normal
#2
@
2 years ago
I came here looking for this! I wanted to add an additional concern/wrinkle. In supporting modules (which as noted can be done with a simple filter). It seems adding support for the `importmap` script type would make this a great addition to the script loader.
This would allow modules to have configurable imports -- enabling sharing modules between front/back ends without having to transpile. I'm sure there are other benefits, but this was the one that sent me looking for that support.
#3
@
20 months ago
- Focuses performance added
- Keywords needs-patch added; 2nd-opinion removed
- Milestone changed from Awaiting Review to Future Release
I think we should consider implementing this. Browser support for this is already in a good place. Having an official way to register modules will allow us to dynamically keep unused javascript from being sent to clients.
Related:
- Gutenberg #36716
- #48654 (Trac)
#6
in reply to:
↑ 4
@
17 months ago
+1
Took me almost one hour to find the needed filter ("script_loader_tag") and the code doesn't look too professional neither.
#7
@
15 months ago
Just a heads up that we have started doing experiments to support modules and import maps:
https://github.com/WordPress/gutenberg/issues/55942
Feel free to review the experiments and share your opinion 🙂
#8
@
15 months ago
- Owner set to luisherranz
- Status changed from new to assigned
I'm assigning the ticket to @luisherranz, as he is actively working on it. Thank you so much for the initiative.
#9
@
15 months ago
- Summary changed from wp_enqueue_script needs to be able to handle modules (ES6/ES2020) to Add the ability to handle ES Modules and Import Maps
#10
@
15 months ago
I've renamed the ticket title to make it more generic as it's not clear yet if this will be something added on top of wp_register_script
or a new API like wp_register_module
.
This ticket was mentioned in PR #5818 on WordPress/wordpress-develop by @luisherranz.
14 months ago
#11
- Keywords has-patch has-unit-tests added; needs-patch removed
Trac ticket: https://core.trac.wordpress.org/ticket/56313
---
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_module
, and wp_enqueue_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_Modules and WP_Scripts
- Dependency Specification:
With
WP_Modules
, the array of dependencies supports not just strings but also arrays that include the dependency type (static
ordynamic
). This design choice allows for future extensions of dependency properties, such as adding aversion
property to support "scopes" within import maps.
- Version Handling:
Versioning in
WP_Modules
accounts forSCRIPT_DEBUG
by default. When in development mode (withSCRIPT_DEBUG
set to true), versions are represented using timestamps, ensuring developers always load the most recent module version during development.
- Module Identifier:
Instead of a handle,
WP_Modules
utilizes the module identifier, aligning with the module identifiers used in JavaScript files and import maps.
- Enqueue and Register Functions:
Unlike
wp_enqueue_script
, thewp_enqueue_module
function does not support registering the script simultaneously. This decision maintains a clear separation of concerns, where registration and enqueuing are distinct actions.
- Deregistration:
There is no equivalent of
wp_deregister_module
at this stage.
## API
### wp_register_module( $module_identifier, $src, $deps, $version )
Registers a module.
// Registers a module with dependencies and versioning.
wp_register_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_module(
'my-module',
'/path/to/my-module.js',
array(
'static-dependency',
array(
'id' => 'dynamic-dependency',
'type' => 'dynamic'
),
)
);
// my-module.js import { ... } from 'static-dependency'; // ... const dynamicModule = await import('dynamic-dependency');
### wp_enqueue_module( $module_identifier )
Enqueues a module.
wp_enqueue_module( 'my-module' );
### wp_dequeue_module( $module_identifier )
Dequeues a module.
wp_dequeue_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 withrel="modulepreload"
attributes. - Lastly, an import map is generated and inserted using a
<script type="importmap">
tag.
## 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.
#12
@
14 months ago
I've opened a PR with a proposal for an initial API. The description and API explanation were copied in the previous comment from GitHub, so I won't repeat them here.
https://github.com/WordPress/wordpress-develop/pull/5818
Please, contribute ideas, potential use cases, or any other thoughts either here or in the PR. Thank you!
This ticket was mentioned in Slack in #core-js by luisherranz. View the logs.
14 months ago
@luisherranz commented on PR #5818:
14 months ago
#14
I see that PHP 8.3 has some deprecations when using ReflectionProperty
. I'll review and fix those.
#16
@
14 months ago
Thanks for the initial patch @luisherranz, a few questions and thoughts that come to me right away:
Can you explain why this reinvents dependency management rather than working off of WP_Dependencies
or _WP_Dependency
?
You mention not having registration and enqueuing together but don't include any rationale behind why extenders of WordPress should need to maintain two different mental models for how external files are loaded into the HTML.
I like the separation of dynamic and static modules, something that I think will be extremely helpful is if there is documentation to help guide developers in making the decision of which one to use.
A useful test would be for circular dependency.
#17
follow-up:
↓ 33
@
14 months ago
Can you explain why this reinvents dependency management rather than working off of
WP_Dependencies
or_WP_Dependency
?
Oh, sure. Overall, I didn't see any benefit in doing so. Reusing WP_Dependencies
resulted in a more complex code because there was practically no overlap as the dependency management we need for modules doesn't require the things that WP_Dependencies
provides (like dependency printing ordering) but requires a few new things, like filtering by type, or, in the future, maintaining more than one dependency per module identifier to support scopes
.
You mention not having registration and enqueuing together but don't include any rationale behind why extenders of WordPress should need to maintain two different mental models for how external files are loaded into the HTML.
This was suggested to me as an improvement over the Scripts API in the past. I don't remember who was at this moment, but I'll ask to see if that person can provide additional rationale.
To me, it 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.
I like the separation of dynamic and static modules, something that I think will be extremely helpful is if there is documentation to help guide developers in making the decision of which one to use.
Absolutely 🙂
A useful test would be for circular dependency.
I'll add one. Thank you!
@luisherranz commented on PR #5818:
14 months ago
#18
Thanks for the review, @swissspidy. I've made all the necessary changes 🙂
@luisherranz commented on PR #5818:
14 months ago
#19
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?
@jonsurrell commented on PR #5818:
14 months ago
#20
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 commented on PR #5818:
14 months ago
#21
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 🙂
13 months ago
#22
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:
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 commented on PR #5818:
13 months ago
#23
@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
?
13 months ago
#24
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.
@flixos90 commented on PR #5818:
13 months ago
#25
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 thefalse
return value does when callingwp_register_script
that tries to register a previously registered script handle.
I agree providing an equivalent method (and wrapper function) to check whether a module has been registered or enqueued would be helpful. Either a wp_module_is( $module, $status )
or two individual wp_module_is_registered( $module )
and wp_module_is_enqueued( $module )
.
Since this class implementation is technically separate from WP_Scripts
and WP_Styles
, I don't think we necessarily have to use the same approach for that. I don't think we explicitly support e.g. queue
or to_do
here anyway. FWIW I think the wp_script_is()
function isn't great from an API perspective with requiring a "status" for something that should rather be distinct functions, so I'd prefer to go with two separate functions, one for registration check and one for enqueue check. Curious what you think.
In any case, if there are any concerns or pushback on this, it could also be added later; not a blocker from my perspective for the initial commit.
13 months ago
#26
In any case, if there are any concerns or pushback on this, it could also be added later; not a blocker from my perspective for the initial commit.
Agreed. Not a blocker for the initial version. Let’s see if wp_module_is_registered()
and friends turn out to be necessary when working on refinements in the Gutenberg plugin to use the version we discuss here. The same applies to wp_deregister_module
. We can always add the polyfill in the Gutenberg plugin if there is no other way to correctly override view modules that WordPress core will ship in the future.
@swissspidy commented on PR #5818:
13 months ago
#27
- Additionally, static dependencies of enqueued modules utilize
link
tags withrel="modulepreload"
attributes.
From the MDN docs:
Preloading allows modules and their dependencies to be downloaded early, and can also significantly reduce the overall download and processing time. This is because it allows pages to fetch modules in parallel, instead of sequentially as each module is processed and its dependencies are discovered. _Note however that you can't just preload everything! What you choose to preload must be balanced against other operations that might then be starved, adversely affecting user experience._
Has any testing been done in this regard?
@luisherranz commented on PR #5818:
13 months ago
#28
Has any testing been done in this regard?
@swissspidy, the static dependencies of the enqueued modules are always required to load those enqueued modules, so the current logic preloads those static dependencies, but not the dynamic ones.
// foo.js import bar from 'bar'; // static dependency, always used, required to load foo.js const baz = await import( 'baz' ); // dynamic dependency, it may or may not be used
This also works recursively, so if bar
has a static dependency, that dependency is also preloaded. This way, we ensure that everything that is required on page load is downloaded in parallel, without causing any waterfalls.
@swissspidy commented on PR #5818:
13 months ago
#29
Greg just brought up the topic of internationalization / localization. Has this been considered for this API here? I imagine people will want to use translations in modules. We'll probably need something similar to wp_set_script_translations
, but also a solution that works well for dynamic dependencies.
@swissspidy commented on PR #5818:
13 months ago
#30
@felixarntz I think we'll want to have a story for localization first , see https://github.com/WordPress/wordpress-develop/pull/5818#issuecomment-1876870705
@flixos90 commented on PR #5818:
13 months ago
#31
That should definitely be a priority, but IMO not a blocker for a first commit.
@luisherranz commented on PR #5818:
13 months ago
#32
Thanks for the thorough review, Weston 🙂
I've addressed all the feedback except the one about renaming the class to WP_Script_Modules
, which I like to discuss a bit more before doing so.
It's very likely that, in the future, once the import attributes are final, we'll add support for importing CSS and JSON:
import sheet from "some-styles-package" with { type: "css" }; document.adoptedStyleSheets = [sheet];
Preloads are a bit different and they can't be enqueued on their own (so that means we would need to allow people to specify the type
), but other than that, they are pretty much like any other module.
It seems like JSON modules are officially referred as JSON modules (link), whereas CSS modules are officially referred as CSS module scripts ([) link] because there was a discussion to rename it from **CSS modules** to avoid confusion with the existing CSS Modules project.
So, I'm not saying that WP_Script_Modules
is worse than WP_Modules
, but that we should also think about the possibility of powering types of modules that are not scripts, and a class name that is suitable for all of them:
- (Java)Script modules
- CSS module scripts
- JSON modules
What do you think?
#33
in reply to:
↑ 17
@
13 months ago
Replying to luisherranz:
You mention not having registration and enqueuing together but don't include any rationale behind why extenders of WordPress should need to maintain two different mental models for how external files are loaded into the HTML.
This was suggested to me as an improvement over the Scripts API in the past. I don't remember who was at this moment, but I'll ask to see if that person can provide additional rationale.
To me, it 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.
Forcing people to keep two different mental models in their head for how to interact with scripts on a page creates an unnecessary burden on the extenders of WordPress.
Why does it need to be two separate steps when it's not anywhere else in WordPress? Enqueuing a module should automatically register it just as enqueuing a script or a style does.
#34
@
13 months ago
If we think long-term, the Scripts API is less relevant because it should be slowly taken over by the Modules API. But it's true that the Styles API is going to stick around and having two mental models will be confusing.
I'll make the necessary changes to the code.
@westonruter commented on PR #5818:
13 months ago
#35
@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 commented on PR #5818:
13 months ago
#36
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.
@Bernhard Reiter commented on PR #5818:
13 months ago
#37
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 commented on PR #5818:
13 months ago
#38
I'd keep it as wp_modules
to keep consistency with the register and enqueue functions.
@Bernhard Reiter commented on PR #5818:
13 months ago
#39
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 😊
@Bernhard Reiter commented on PR #5818:
13 months ago
#42
Committed to Core in https://core.trac.wordpress.org/changeset/57269.
Thank you, folks! This is big 🎉
@luisherranz commented on PR #5818:
13 months ago
#43
Follow-up tickets:
- Import map polyfill: https://core.trac.wordpress.org/ticket/60232
- Support for
editorModule
/module
/viewModule
inblock.json
: https://core.trac.wordpress.org/ticket/60233 - Translations API: https://core.trac.wordpress.org/ticket/60234
@flixos90 commented on PR #5818:
13 months ago
#44
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.
#46
follow-up:
↓ 51
@
13 months ago
- Keywords commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
Nice work on this everyone! I'll reopen this ticket for some follow-up enhancements. Some of these may warrant separate tickets.
- The naming consideration raised in the discussion on https://github.com/WordPress/wordpress-develop/pull/5818
- This feature only works on the front end, not within the wp-admin area, is this intentional?
- Is it desirable or possible for a module to depend on a non-module script, or vice versa? Should it be?
- There appears to be no way to fetch the list of enqueued modules like it is for non-module scripts. I'd like to display the list of enqueued modules in my Query Monitor plugin but it doesn't seem possible. Perhaps the
get_marked_for_enqueue()
andget_dependencies()
methods need to be public instead of private.
@westonruter commented on PR #5818:
13 months ago
#47
IMO, I think the functions should be wp_*_script_module()
for consistency.
13 months ago
#48
IMO, I think the functions should be
wp_script_modules()
,wp_register_script_module()
, andwp_enqueue_script_module()
for consistency.
It makes perfect sense after the class name changes were applied.
#49
@
13 months ago
Is it desirable or possible for a module to depend on a non-module script, or vice versa? Should it be?
I think so, yes.
Copy-pasting here the opinion I shared about this topic in the Gutenberg Tracking Issue:
Migrating from scripts to modules is not possible for scripts that have external dependants due to backward compatibility issues. This means there are going to be scripts that will remain scripts forever.
To increase the adoption of modules, it makes sense that:
- New modules can still use those remaining scripts as dependencies. Ideally, developers should be able to use modules instead of scripts, and the need for script dependencies should not be an obstacle to the decision to use modules instead of scripts.
- Those remaining scripts can use new modules as dependencies. Ideally, all new packages should be modules from now on, and the need for existing scripts to use those modules as dependencies should not be an obstacle to the decision to use modules instead of scripts.
I also believe that the priority should be on the modules:
- From now on, WordPress developers should be able to use modules instead of scripts for all their needs.
- All the existing WordPress packages should be available to be consumed as modules from now on, even if some of them are linked to existing scripts under the hood.
- Developers should not need to know which WordPress packages are linked to existing scripts. For them, everything should look like regular modules from now on.
- Developers should not need to use a _script-related_ bundling tool while building their modules anymore.
- Existing scripts should be able to import modules, but in this case, the API can be explicit: developers should know when they are importing a module instead of a script.
There appears to be no way to fetch the list of enqueued modules like it is for non-module scripts. I'd like to display the list of enqueued modules in my Query Monitor plugin but it doesn't seem possible. Perhaps the
get_marked_for_enqueue()
andget_dependencies()
methods need to be public instead of private.
As stated in the PR description, this initial version was intentionally simplistic. That way, we start with something that provides the basic functionality, and build upon it incrementally. So if you want/need to add new capabilities, I think those should be discussed in new tickets.
This feature only works on the front end, not within the wp-admin area, is this intentional?
It's not. Feel free to add a new ticket to add hooks for the admin.
13 months ago
#50
I think the functions should be wp_script_modules(), wp_register_script_module(), and wp_enqueue_script_module() for consistency.
Another +1 for this.
#51
in reply to:
↑ 46
@
13 months ago
Replying to johnbillion:
- The naming consideration raised in the discussion on https://github.com/WordPress/wordpress-develop/pull/5818
Yeah, this needs fixing. Thinking these https://github.com/WordPress/wordpress-develop/pull/5818#issuecomment-1887867378 are quite better.
- Is it desirable or possible for a module to depend on a non-module script, or vice versa? Should it be?
I'm "on the fence" on this but thinking perhaps not for now. A JS module depending on jQuery? Seems a bit like like mixing apples with oranges? :) Really want to see some "real life" examples of how that would work, and how useful it may be.
Also should this even be needed in PHP or can it be done better in the browser/with JS? Modules are loaded as either defer
or async
, so any hard-coded "old-style" JS (that is outputted before any modules in the HTML head) will always be available to them.
@luisherranz commented on PR #5818:
13 months ago
#52
Seems unanimous, then. I'll prepare a PR with the renaming.
This ticket was mentioned in PR #5869 on WordPress/wordpress-develop by @luisherranz.
13 months ago
#53
Trac ticket: https://core.trac.wordpress.org/ticket/56313
---
## What
Rename the public wp...module
functions to wp...script_module
:
wp_module()
->wp_script_module
wp_register_module()
->wp_register_script_module
wp_enqueue_module()
->wp_enqueue_script_module
wp_dequeue_module()
->wp_dequeue_script_module
## Why
Because it was decided after the patch was already committed:
@luisherranz commented on PR #5818:
13 months ago
#54
@luisherranz commented on PR #5869:
13 months ago
#55
Sure 🙂
Do you want me to remove the module
word from the print methods as well?
print_enqueued_modules
->print_enqueued
print_module_preloads
->print_preloads
Or maybe change them to script_module
instead?
print_enqueued_modules
->print_enqueued_script_modules
print_module_preloads
->print_script_module_preloads
@flixos90 commented on PR #5869:
13 months ago
#56
@luisherranz I think the latter, let's use script_module(s)
everywhere.
@luisherranz commented on PR #5869:
13 months ago
#57
I've renamed all the references to "modules" and replaced them with "script modules".
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
13 months ago
@luisherranz commented on PR #5869:
13 months ago
#59
Suggested edits to phpdoc and added PHP 7 typing
Thanks, @westonruter. I've applied all the necessary changes.
@luisherranz commented on PR #5869:
13 months ago
#60
Ok, everything looks good now. Thanks for your help, @westonruter 🙂
The PR is ready.
#61
@
13 months ago
FYI: I just filed #60320 to fix corrupted importmap JSON when HTML5 theme support is absent.
#66
@
13 months ago
Follow-up PR and Trac ticket to move the prints to the footer in classic themes:
This ticket was mentioned in PR #5945 on WordPress/wordpress-develop by @swissspidy.
13 months ago
#69
Only noticed this while looking at https://github.com/WordPress/wordpress-develop/pull/5888
wp_script_modules()
is an odd function because a) it doesn't use a get_instance()
method like elsewhere in core, b) it does more than just returning an instance. The first time you call it, it adds hooks.
Curious to hear thoughts on this alternative suggestion.
cc @luisherranz
Trac ticket: https://core.trac.wordpress.org/ticket/56313
@luisherranz commented on PR #5945:
13 months ago
#70
The idea of wp_script_modules()
came from @felixarntz in these comments of the original PR:
I would recommend to make everything here non-static and instead introduce a simple
wp_modules()
function that encapsulates the default/main instance of the class, similar to how the existingwp_scripts()
andwp_styles()
functions do.
The idea to avoid loading/instantiating the class early in the WordPress bootstrap also came from Felix:
It would be great if you could introduce wrapper functions for these 3 as well (i.e.
wp_print_import_map()
etc.). This way you can reference the functions here, which avoids loading/instantiating the class early in the WordPress bootstrap process where it may not be needed.
Not a big deal, but an easy enough optimization IMO worth making.
Taking into account what Felix said, I thought it would be even nicer to add the hooks inside the first call to the wp_script_modules()
function. That not only prevents the hooks from being added early in the WordPress bootstrap, but doesn't add them at all if nobody is using modules.
Again, as Felix said, it's probably not a big deal, just an optimization.
I don't have strong preferences over any of the options, though, so feel free to change it if you want 🙂
@swissspidy commented on PR #5945:
13 months ago
#71
Hmm I see.
Well in that case I suppose keeping the function is fine, but not a fine of adding the hooks in there as well.
sheebaawp commented on PR #5945:
13 months ago
#72
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
13 months ago
@swissspidy commented on PR #5945:
13 months ago
#75
Committed in https://core.trac.wordpress.org/changeset/57503
#76
@
13 months ago
- Keywords commit removed
- Resolution set to fixed
- Status changed from reopened to closed
@luisherranz commented on PR #5945:
13 months ago
#77
Could you please take a look at the class initialization at https://github.com/WordPress/wordpress-develop/pull/5953? I'm doing something similar there to what I did here, and I don't know what the correct way to do it would be.
Thanks 🙏
#79
follow-up:
↓ 80
@
12 months ago
There is now a way to deregister a script module added with #60463:
<?php function wp_deregister_script_module( string $id ) { wp_script_modules()->deregister( $id ); }
The question is, should we call it wp_deregister_script_module
for consistency with scripts and styles. If we want to keep consistency, should we also rename all $id
params with $handle
and deregister()
method with remove()
?
#80
in reply to:
↑ 79
;
follow-up:
↓ 82
@
12 months ago
Replying to gziolo:
The question is, should we call it
wp_deregister_script_module
for consistency with scripts and styles.
Imho this name is descriptive, sounds good, and seems consistent. Replacing deregister
with remove
would also work (imho both are quite good).
If we want to keep consistency, should we also rename all
$id
params with$handle
Yea $handle
is used everywhere in (the old) script loader. However I've always had the feeling it is somewhat unclear what exactly it represents: something between "name" and "id"?
Agree that the current $id
params may be a bit ambiguous. They were renamed in [57327] from $module_id
. If they are renamed again, I'd suggest using the old name: $module_id
as that seems better than $handle
imho. Another possibility would be $script_module_id
which is long but best describes what they are and how they are used.
This ticket was mentioned in PR #6160 on WordPress/wordpress-develop by @cbravobernal.
12 months ago
#81
Changes script module deregister internal function name to remove
, to keep consistency with the other scripts and styles functions.
Trac ticket: https://core.trac.wordpress.org/ticket/56313
#82
in reply to:
↑ 80
@
12 months ago
Replying to azaozz:
Imho this name is descriptive, sounds good, and seems consistent. Replacing
deregister
withremove
would also work (imho both are quite good).
I created a small PR to change that: https://github.com/WordPress/wordpress-develop/pull/6160
Agree that the current
$id
params may be a bit ambiguous. They were renamed in [57327] from$module_id
. If they are renamed again, I'd suggest using the old name:$module_id
as that seems better than$handle
imho. Another possibility would be$script_module_id
which is long but best describes what they are and how they are used.
If we consider $id more descriptive than $handler, imho, adding the script_module
may not be necessary, as those functions are already inside a script-modules folder. (Although it is true that is not the best for search indexes).
But, while changing that, we could mess up with the dependencies array, which all of them are currently using 'id' key as reference.
<?php // src/wp-includes/class-wp-script-modules.phpL73 $dependencies[] = array( 'id' => $dependency ['id'] , 'import' => isset( $dependency['import'] ) && 'dynamic' === $dependency['import'] ? 'dynamic' : 'static', );
#84
@
12 months ago
Regarding the $id
parameter, $module_id
or $script_module_id
removes and doubt and is likely easier to find when searching. However, in the context of modules $id
seems unlikely to be a significant source of confusion. My preference would be to maintain $id
.
For precedent, scripts use $handle
, not $script_handle
.
<?php function wp_register_script( $handle /*…*/ ) {} function wp_register_script_module( string $id /*…*/ ) {}
Although it may be a poor comparison 🙂 I agree with azaozz that
[
$handle
] is somewhat unclear what exactly it represents: something between "name" and "id"?
@jonsurrell commented on PR #6160:
9 months ago
#85
This doesn't seem necessary at this point, especially now that it would change a public function that was released in 6.5.
Hi @idad5, welcome back to Trac!
I think it makes sense that this is in Core territory rather than a third-party plugin. This should be reasonably straightforward. - it can even just be done through filters:
For now, I'll add the
2nd-opinion
keyword to get some thoughts on the enhancement from other contributors.