[AssetMapper] Add a "package specifier" to importmap in case import name != package+path #52024
Conversation
|
|
||
| return Command::FAILURE; | ||
| } | ||
| } |
There was a problem hiding this comment.
Check is done in ImportMapManager with a nice error
| // https://regex101.com/r/5Q38tj/1 | ||
| private const IMPORT_PATTERN = '/(?:import\s+(?:(?:\*\s+as\s+\w+|[\w\s{},*]+)\s+from\s+)?|\bimport\()\s*[\'"`](\.\/[^\'"`]+|(\.\.\/)*[^\'"`]+)[\'"`]\s*[;\)]?/m'; | ||
| // https://regex101.com/r/fquriB/1 | ||
| private const IMPORT_PATTERN = '/(?:import\s*(?:(?:\*\s*as\s+\w+|[\w\s{},*]+)\s*from\s*)?|\bimport\()\s*[\'"`](\.\/[^\'"`]+|(\.\.\/)*[^\'"`]+)[\'"`]\s*[;\)]?/m'; |
| } | ||
|
|
||
| return $assetMapper->getAsset($importMapEntry->path); | ||
| return $assetMapper->getAssetFromSourcePath($importMapEntry->path); |
There was a problem hiding this comment.
e.g. bootstrap imports @popperjs/core. If bootstrap is ultimately preloaded, this is what allows us to also preload @popperjs/core.
|
|
||
| $packageVersion = $entry->importName.($version ? '@'.$version : ''); | ||
| $packageAudits[$packageVersion] ??= new ImportMapPackageAudit($entry->importName, $version); | ||
| $packageVersion = $packageName.'@'.$version; |
There was a problem hiding this comment.
Bug fix - e.g. if you have chart.js/auto, $packageName will now be just chart.js, which is what we need for the API call. Covered with a test.
Also, $version is now guaranteed not null here.
| @@ -38,7 +40,7 @@ public function getEntries(): ImportMapEntries | |||
|
|
|||
| $entries = new ImportMapEntries(); | |||
| foreach ($importMapConfig ?? [] as $importName => $data) { | |||
| $validKeys = ['path', 'version', 'type', 'entrypoint', 'url']; | |||
| $validKeys = ['path', 'version', 'type', 'entrypoint', 'url', 'package_specifier']; | |||
There was a problem hiding this comment.
Supports the rare-use case that you require a package (e.g. bootstrap) but want to have its import called something else - shoestrap - so you can import 'shoestrap'. The importmap:require command supports this directly & it's documented.
| public static function createRemote(string $importName, ImportMapType $importMapType, string $path, string $version, string $packageModuleSpecifier, bool $isEntrypoint): self | ||
| { | ||
| return new self($importName, $importMapType, $path, $isEntrypoint, $version, $packageModuleSpecifier); | ||
| } |
There was a problem hiding this comment.
private constructor + static factories. Done this way so that IF you are remote, we can guarantee that both $version and $packageModuleSpecifier ARE included.
| } | ||
| $asset = $this->findAsset($entry->path); | ||
| if (!$asset) { | ||
| throw $this->createMissingImportMapAssetException($entry); |
There was a problem hiding this comment.
Example of why it's nice now that ImportMapEntry.path is populated even for vendor/remote assets :)
| // remote packages aren't in the asset mapper & so don't have dependencies | ||
| if ($entry->isRemotePackage()) { | ||
| return $currentImportEntries; | ||
| } |
There was a problem hiding this comment.
An edge case (that does not currently apply) where vendor assets have relative imports, so we need to add those to the importmap.
More broadly, just another example of not treating the remote packages any differently.
src/Symfony/Component/AssetMapper/ImportMap/ImportMapConfigReader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/ImportMapConfigReader.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/ImportMapManager.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/ImportMapUpdateChecker.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/RemotePackageStorage.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Tests/Command/DebugAssetsMapperCommandTest.php
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/RemotePackageStorage.php
Outdated
Show resolved
Hide resolved
d40ed00 to
a0b1e27
Compare
…ame != package+path
|
Thank you @weaverryan. |
a0b1e27 to
c80a1ab
Compare
Hi!
Sorry for the large PR - this tightens up the component after the many recent enhancements.
tl;drMakes "vendor" importmap entries work identically to "local" entries when it comes to preloading (a bug). Also tightens upImportMapEntry, which is a tricky class because if an entry is "remote", it has 2 extra, required, properties.Full changes:
Cheers!