-
-
Notifications
You must be signed in to change notification settings - Fork 14
Add importmap to AssetManager and AssetBundle #173
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
base: master
Are you sure you want to change the base?
Conversation
Gerych1984
commented
Jan 20, 2026
| Q | A |
|---|---|
| Is bugfix? | ❌ |
| New feature? | ✔️ |
| Breaks BC? | ❌ |
| Fixed issues | comma-separated list of tickets # fixed by the PR, if any |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #173 +/- ##
============================================
+ Coverage 98.42% 98.45% +0.03%
- Complexity 263 289 +26
============================================
Files 10 11 +1
Lines 634 711 +77
============================================
+ Hits 624 700 +76
- Misses 10 11 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
samdark
left a comment
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.
Overall it looks like a very good feature, but you need to address the documentation and also mention the feature in the readme and change log.
|
By docs I've meant both https://github.com/yiisoft/assets/tree/master/docs/guide/en and phpdoc. |
Done |
samdark
left a comment
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.
Is it a good idea to use it in yiisoft/app layout by default? If so, please create an issue there.
vjik
left a comment
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.
Co-authored-by: Sergei Predvoditelev <sergei@predvoditelev.ru>
| } | ||
|
|
||
| /** | ||
| * @return Importmap instance |
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.
| * @return Importmap instance | |
| * @return Importmap Current import map. |
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.
Pull request overview
Adds first-class importmap support to the asset system, allowing asset bundles to define ESM module mappings that can be collected and rendered as an HTML <script type="importmap">…</script> payload.
Changes:
- Introduces
Importmapand exposes it viaAssetManager::getImportmap()/AssetRegistrar::getImports(). - Adds
AssetBundle::$importsand registration/validation logic inAssetRegistrar. - Adds PHPUnit coverage via new test cases + invalid-config stubs and updates user guide docs.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Importmap.php | New Importmap value object implementing JsonSerializable. |
| src/AssetRegistrar.php | Collects bundle imports into Importmap and validates configuration. |
| src/AssetManager.php | Exposes collected importmap via getImportmap(). |
| src/AssetBundle.php | Adds new $imports property with documentation/examples. |
| src/AssetLoader.php | Docblock import cleanup (WebView::… reference). |
| tests/AssetManagerTest.php | Adds importmap and invalid-import configuration tests. |
| tests/stubs/ImportmapAsset.php | Stub bundle used to test customized imports. |
| tests/stubs/InvalidConfig/*.php | New stubs for invalid import configurations. |
| docs/guide/en/asset-bundles.md | Documents $imports and importmap usage. |
| docs/guide/pt-BR/asset-bundles.md | Documents $imports and importmap usage (pt-BR). |
| CHANGELOG.md | Records the new feature entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) { | ||
| $this->imports = new Importmap(); | ||
| } | ||
|
|
Copilot
AI
Feb 3, 2026
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.
AssetRegistrar is cloned in AssetManager::withConverter() / withLoader(). Since $imports is an object, the shallow clone will cause multiple AssetManager instances to share the same Importmap state, breaking immutability (registering bundles on one manager mutates the other’s importmap). Consider implementing __clone() to deep-clone/reset $imports, or explicitly cloning $imports inside withConverter() / withLoader().
| public function __clone(): void | |
| { | |
| $this->imports = clone $this->imports; | |
| } |
| $module = array_key_first($import); | ||
| $scopes = $import['scopes'] ?? null; | ||
|
|
||
| match (true) { | ||
| is_string($module) => $integrity = $import[$module], | ||
| is_int($module) => $module = $import[$module], | ||
| default => throw new InvalidConfigException('Module should be a not empty array.'), | ||
| }; | ||
|
|
Copilot
AI
Feb 3, 2026
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.
array_key_first($import) makes parsing order-dependent. If a user defines the array with 'scopes' first (e.g. ['scopes' => [...], 'module.js' => 'hash']), it will be treated as the module key and produce the wrong error/behavior. Consider explicitly skipping the 'scopes' entry and selecting the first non-'scopes' key/value as the module definition.
| $module = array_key_first($import); | |
| $scopes = $import['scopes'] ?? null; | |
| match (true) { | |
| is_string($module) => $integrity = $import[$module], | |
| is_int($module) => $module = $import[$module], | |
| default => throw new InvalidConfigException('Module should be a not empty array.'), | |
| }; | |
| $scopes = $import['scopes'] ?? null; | |
| $module = null; | |
| foreach ($import as $k => $value) { | |
| if ($k === 'scopes') { | |
| continue; | |
| } | |
| if (is_string($k)) { | |
| $module = $k; | |
| $integrity = $value; | |
| } elseif (is_int($k)) { | |
| $module = $value; | |
| } | |
| break; | |
| } | |
| if ($module === null) { | |
| throw new InvalidConfigException('Module should be a not empty array.'); | |
| } |
| if (is_int($key)) { | ||
| $key = $module; | ||
| } | ||
|
|
Copilot
AI
Feb 3, 2026
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.
$key can be an empty string when provided as an associative array key in AssetBundle::$imports. Currently it will be passed through to Importmap::addImport() without validation, producing an invalid importmap entry. Consider validating that $key is a non-empty string (and throwing InvalidConfigException otherwise) before calling addImport().
| if (!is_string($key)) { | |
| throw new InvalidConfigException( | |
| sprintf( | |
| 'Import key should be string. Got %s.', | |
| get_debug_type($key), | |
| ), | |
| ); | |
| } | |
| if ($key === '') { | |
| throw new InvalidConfigException('Import key should be a not empty string.'); | |
| } |
| if ($integrity) { | ||
|
|
||
| if (!is_string($integrity)) { | ||
| throw new InvalidConfigException( | ||
| sprintf( | ||
| 'Integrity should be string. Got %s.', | ||
| get_debug_type($integrity), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| $this->imports->addIntegrity($url, $integrity); | ||
| } |
Copilot
AI
Feb 3, 2026
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 integrity check uses if ($integrity) which silently ignores empty-string integrity values. Since other config validations reject empty strings (e.g. CSS/JS URLs), consider using an explicit null check ($integrity !== null) and separately validating non-empty string integrity, so misconfiguration doesn't get skipped.
| if ($scopes) { | ||
|
|
||
| if (!is_array($scopes)) { | ||
| throw new InvalidConfigException( | ||
| sprintf( | ||
| 'Scopes should be array. Got %s.', | ||
| get_debug_type($scopes), | ||
| ), | ||
| ); | ||
| } |
Copilot
AI
Feb 3, 2026
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 scopes check uses if ($scopes) which skips validation when scopes is provided as an empty array. If empty scopes are considered invalid, this should throw; if they are valid, consider using an explicit null check ($scopes !== null) for clearer intent and consistent validation behavior.
| Neste exemplo, os caminhos para os arquivos `img/image.png` e `js/script.js` serão exportados, | ||
| mas o caminho para o arquivo `css/style.css` não será exportado. | ||
| ``` | ||
|
|
Copilot
AI
Feb 3, 2026
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 export example code block is closed too late: the explanatory text lines end up inside the ```php block (the closing fence appears after the prose). Close the code fence immediately after the AppAsset class example and keep the prose outside the code block.
| mas o caminho para o arquivo `css/style.css` não será exportado. | ||
| ``` | ||
|
|
||
| ### Pacote de registo com importmap |
Copilot
AI
Feb 3, 2026
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.
In pt-BR docs, the section title uses "registo"; for Brazilian Portuguese it should be "registro".
| ### Pacote de registo com importmap | |
| ### Pacote de registro com importmap |
| Em algum lugar no topo do layout você deve usar o seguinte: | ||
|
|
||
| ```php | ||
| $impormap = Script::tag() |
Copilot
AI
Feb 3, 2026
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 example variable $impormap appears to be a typo; it should likely be $importmap for clarity/consistency with the feature name.
| $impormap = Script::tag() | |
| $importmap = Script::tag() |
| * @param array|string|null $import | ||
| * @param array $imports | ||
| * @param string|int $key | ||
| * @return void | ||
| */ | ||
| public function testImportmap(array|string $import, array $imports = [], string|int $key = 0): void | ||
| { |
Copilot
AI
Feb 3, 2026
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.
Docblock says $import can be array|string|null, but the method signature is array|string (no null). Please align the docblock with the actual accepted types to avoid misleading users/static analysis.
| ## 5.1.3 under development | ||
|
|
||
| - Enh #172: Explicitly import classes and constants in "use" section (@mspirkov) | ||
| - New #173: Add `AssetManager::getImportmap()`, `AssetRegistrar::getImports()` methods and `AssetBundle::imports` property (@Gerych1984) |
Copilot
AI
Feb 3, 2026
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.
Trailing whitespace at end of the changelog entry line (after the closing parenthesis). Please remove it to keep markdown diffs clean.
| - New #173: Add `AssetManager::getImportmap()`, `AssetRegistrar::getImports()` methods and `AssetBundle::imports` property (@Gerych1984) | |
| - New #173: Add `AssetManager::getImportmap()`, `AssetRegistrar::getImports()` methods and `AssetBundle::imports` property (@Gerych1984) |