Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Script Modules API #5818
Changes from 13 commits
8347e8b
27ba624
156eb58
bbae17c
76e8726
f0fa726
f13a068
ef978d8
212cdac
e644da8
1aa45a6
eaaa563
e30c8e9
a13bc87
67f3900
66987f0
fda294d
c94568d
2eaf6ea
4dcfc88
a4f9812
89a82fc
9f08176
4b815bc
bc8c633
b3554d6
35a9402
df73485
8fd7501
506784a
168ae91
1df672d
0cb7f1b
834693d
b2ff803
adb5913
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can plugins override the registered module? It would be great to add a unit test that covers that use case, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They currently can't. Do you think it's necessary for the initial version?
If so, what would be the ideal API to override a module registration?
I'm not familiar with the use case, but to me, the fact that you need to deregister the script to override it it's not very straightforward.
Or alternatively:
I'd rather expose a more explicit API, but curious to hear other's opinions 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for only allowing to "override" by deregistering the module explicitly first. That is also the way required for the existing core APIs for scripts and stylesheets.
I think an explicit API makes it less error-prone, allowing to simply override could lead to accidental overrides too easily, while by requiring deregistration first it'll surely be intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about this during the last few days, and I don't think adding a deregister method is a good idea at this moment.
First, the use cases for that API are indirect:
There is no clear direct use case for deregistering a module.
But the main reason is that there can be multiple sources for the same module identifier:
That's a very powerful feature that we should study how to leverage in the future.
If, at some point, we allow
wp_register_module
to register different modules for the same module identifier, it's not clear which onewp_deregister_module
should deregister.At least, I'd rather merge this initial version without it, and wait to see what other options we have to solve the use cases that are presented when we start integrating it.
Some examples of how we could leverage `scopes` that come to my mind.
Support different versions through semantic versioning
Compat adapters for blocks using old versions
Translation APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: we need to move these prints to the footer on classic themes (related Gutenberg PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution used in Gutenberg was just temporary because there's no need to print all the modules (or preloads) at once. So taking into account that the order in which modules are printed is not important, and that the sooner they are on the page the better because the browser will start downloading them sooner:
wp_head
andwp_footer
. They will print the modules that are already available in thewp_head
(like modules of blocks in block themes, or modules from classic themes or plugins) and they will print again the modules that were not available in thewp_head
but are available in thewp_footer
(like modules of blocks in classic themes).This is the explanation included in the code:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c4rl0sbr4v0 discovered that we can't enqueue or preload any module before we know all the dependencies because the import map needs to be added before any module:
To be honest, this is quite a surprise, especially the preload part. If someone knows better, please tell so.
I've already opened a Trac ticket to fix it: https://core.trac.wordpress.org/ticket/60240