Conversation
Introduces a `scopes` arg on `wp_register_script_module()` and `wp_enqueue_script_module()` that constrains which importers can resolve a module's bare specifier. Modules with `scopes` are emitted under the import map's top-level `scopes` field, not `imports`. Each `scopes` entry is one of: - A non-empty URL prefix string, emitted as-authored. - `array( 'module_id' => $id )`, resolved at print time to the directory of the named module's filtered src URL (CDN-aware via the existing `script_module_loader_src` filter). `scopes => array()` registers a module that is not resolvable via bare specifier from anywhere; static dependencies on such a module are treated like missing dependencies. This is API hygiene + bare-specifier scoping per the import map spec — not a security boundary. Files remain fetchable via direct URL `import()`; resolution failure for out-of-scope importers is a runtime `TypeError`.
…d modules. Address review feedback for the scoped script modules feature: - Stop import-map traversal at modules registered with empty scopes so their public transitive dependencies don't leak into top-level `imports`. - Warn when a classic script's `module_dependencies` reference a module registered with empty scopes; such a dependency cannot be resolved via bare specifier from the classic script. - Warn when a `module_id` scope resolves to a URL with no path separator (which cannot be reduced to a directory). - Improve register-time validation messages to include the module identifier and received type, and avoid PHP syntax in translatable strings. - Correct the docblock claim that only static dependencies on empty-scoped modules fail; declared dependencies (static or dynamic) are treated like missing dependencies, mirroring existing missing-dep semantics. - Tighten test assertions and wrap a filter in `try/finally` to prevent leaks between tests; add coverage for the transitive-leak fix, the classic-script empty-scoped warning, and the `module_id` no-slash diagnostic.
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @gemini-code-assist. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements "scopes" support for script modules, enabling private module registration and scoped resolution within the import map. Key changes include updating the register and enqueue methods to handle the scopes parameter, implementing validation logic, and refining the dependency graph traversal to respect module visibility. Feedback identifies an optimization opportunity in the dependency queue processing and a potential bug in URL path resolution for scope keys when encountering bare domains.
| * walking through them would leak otherwise-private deps into top-level `imports`. | ||
| */ | ||
| $collected_dependencies = array(); | ||
| $id_queue = array_merge( $this->queue, $classic_script_module_dependencies ); |
There was a problem hiding this comment.
The $id_queue is initialized by merging $this->queue and $classic_script_module_dependencies. If an ID exists in both arrays, it will be processed twice in the subsequent while loop. While the impact is likely small given the typical number of modules, using array_unique here is a simple optimization to avoid redundant iterations.
$id_queue = array_unique( array_merge( $this->queue, $classic_script_module_dependencies ) );|
|
||
| // Strip query and fragment, truncate to last "/" to derive directory. | ||
| $path = preg_replace( '~[?#].*$~', '', $other_src ); | ||
| $last_slash = strrpos( (string) $path, '/' ); |
There was a problem hiding this comment.
When resolving a module_id scope, strrpos searches for the last slash in the URL. If the URL is a bare domain (e.g., https://example.com), it will find the slash in the protocol, resulting in an incorrect and overly broad scope key like https://. Consider verifying that the found slash is part of the path component and not the protocol.
A module registered with `scopes => array()` and enqueued is an entry point: it is emitted as a `<script type="module">` and its imports must resolve in the browser even though its own bare specifier resolves nowhere. The dependency walk in `get_import_map()` previously stopped at every empty-scoped node, including the starting node, leaving the entry's deps absent from the import map. Stop traversal at *transitive* empty-scoped nodes only. Starting nodes (queue items and classic-script `module_dependencies`) are now exempt. The leak prevention for empty-scoped modules reached only via dep edges is preserved.
1 participant
Trac ticket:
Use of AI Tools
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.