Script Modules: Add method to include modules in importmap #8009
Conversation
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
@luisherranz I'd love your thoughts on this. @youknowriad @swissspidy This is one of the first pieces towards allowing scripts to depend on modules, I know that you're working on functionality that would like to leverage this. I'd appreciate your thoughts. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| * | ||
| * @param string $id The identifier of the script module. | ||
| */ | ||
| public function include_in_import_map( string $id ) { |
There was a problem hiding this comment.
I wonder if this method name is too specific and tied to the implementation.
Originally I had used expose() and was also considering make_available(). I'm wondering about a future where this may add some option arguments for preload, etc.
There was a problem hiding this comment.
My first thought was add_to_import_map()
|
I added a test and fixed an issues where regular dependencies and "requested for inclusion" modules were being incorrectly overwritten. |
This allows for script modules to be exposed in the import map regardless of being a dependency or enqueued. This will enable scripts to depend on script modules by requesting they be exposed.
The + operator merges arrays with key overwriting. array_merge renumbers numeric arrays, the desired behavior in this case.
sirreal
force-pushed
the
script-modules/new-method--add-to-importmap
branch
from
350705f to
d30c9de
Compare
| /** | ||
| * @ticket 61500 | ||
| */ | ||
| public function test_included_module_appears_in_importmap() { |
There was a problem hiding this comment.
In core, the best practice when using multiple assertions in a test is for each assertion to have a custom error message. Makes debugging easier.
|
#8024 implements a complete system utilizing this. I'm not too happy with the result. It requires scripts to print before the importmap which is not reliable. It may be better to have scripts continue to declare module dependencies, but when printing the import map have the module system query enqueued scripts and print their module dependencies in the importmap. I'll make this PR a draft while I continue to explore a complete implementation. |
Code-wise, it looks good to me. The only question I have is why you decided to use an external array instead of a flag in the properties of the registered script module (like |
|
I've found an approach in #8024 that seems like a better solution and does not require this work. I'll close this PR. If folks think there's still value in adding this method we can reopen and consider it. |
This is a first step towards 61500, it adds a method to the script modules class to allow modules to be flagged for inclusion in the import map.
This means that a module can be flagged for inclusion and it will appear in the import map regardless of whether it's absent from the script module dependency graph.
This can be useful for developers to force modules to appear in the import map where they can be imported on the browser console. I expect this method to be used in the future so that classic scripts can create a dependency on script modules, where this new method will act the script dependency system to request that modules appear in the importmap.
Internally it uses an array:moduleId => null. The value is always null, but other values could be used, e.g. to request that it appear in the import map and in module preloads the value might bymoduleId => trueormoduleId => 'preload'. Onlynullis used at this time. This would also appear as an argument to theinclude_in_import_map` method, but again that is not implemented at this time and only presented for consideration.Trac ticket: https://core.trac.wordpress.org/ticket/61500
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.