-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
[AssetMapper] Change default importmap "provider" to JsDelivr+esm #50408
Conversation
Hey! Thanks for your PR. You are targeting branch "6.4" but it seems your PR description refers to branch "6.3". Cheers! Carsonbot |
4e90dbf
to
cdba8e7
Compare
The "high-deps" failure is because it's trying to pull in the latest commit from |
The failing high-deps is because your Pr does not target the highest version (as the 6.4 branch has been created). It is fine to ignore that failure as it will be resolved once your merged PR is merged up from 6.3 to 6.4. |
src/Symfony/Component/AssetMapper/ImportMap/Providers/JsDelivrEsmImportMapProvider.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Tests/ImportMap/ImportMapManagerTest.php
Outdated
Show resolved
Hide resolved
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.
Awesome!!
src/Symfony/Component/AssetMapper/ImportMap/Providers/JsDelivrEsmImportMapProvider.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/Providers/JsDelivrEsmImportMapProvider.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/Providers/JspmImportMapProvider.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/Providers/ResolvedImportMapPackage.php
Outdated
Show resolved
Hide resolved
70defcf
to
f624325
Compare
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.
|
||
$container | ||
->getDefinition('asset_mapper.importmap.resolver') | ||
->replaceArgument(0, $config['provider']) |
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.
not processing the value at compile-time is important to make this configurable by env var
; | ||
|
||
$container | ||
->getDefinition('asset_mapper.importmap.renderer') | ||
->replaceArgument(2, $config['importmap_polyfill'] ?? ImportMapManager::POLYFILL_URL) | ||
->replaceArgument(3, $config['importmap_script_attributes']) | ||
; | ||
|
||
$container->registerForAutoconfiguration(PackageResolverInterface::class) | ||
->addTag('asset_mapper.importmap.resolver'); |
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.
this new tag allows defining custom package resolvers in case one wants to host their own
public function __construct( | ||
HttpClientInterface $httpClient = null, | ||
private readonly string $versionUrlPattern = self::URL_PATTERN_VERSION, | ||
private readonly string $distUrlPattern = self::URL_PATTERN_DIST, |
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 made those URLs configurable to make it possible to reuse the code while self-hosting somewhere else.
} | ||
|
||
$response = $this->httpClient->request('GET', sprintf($this->versionUrlPattern, $packageName, urlencode($constraint))); | ||
$requiredPackages[] = [$options, $response, $packageName, $filePath, $options]; |
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.
by stacking HTTP requests, we can fetch versions concurrently
} | ||
|
||
$version = $response->toArray()['version']; | ||
$requiredPackages[$i][1] = $this->httpClient->request('GET', sprintf($this->distUrlPattern, $packageName, $version, $filePath)); |
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.
and we can also concurrently fetch package dists
$resolvedPackages = []; | ||
foreach ($response->toArray()['map']['imports'] as $packageName => $url) { | ||
$options = $packageRequiresByName[$packageName] ?? new PackageRequireOptions($packageName, null, $defaultOptions->download, $defaultOptions->preload); | ||
$resolvedPackages[] = [$options, $url, $options->download ? $this->httpClient->request('GET', $url, ['base_uri' => $this->baseUri]) : null]; |
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.
fetch contents from jspm concurrently also
{ | ||
} | ||
|
||
public function getLogicalPath(): string |
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.
replacing most getters+setters by readonly props (except for "dependencies" arrays, which need mutability)
f624325
to
2cf9176
Compare
… readonly properties in MappedAsset
2cf9176
to
b530dc3
Compare
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.
Looks great - I checked the changes, I don't see any issues. It's more polished - you accomplished the immutability with MappedAsset
, which I couldn't quite pull off. Thank you!
} | ||
|
||
return array_values($resolvedPackages); | ||
} |
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.
😳
Thank you @weaverryan. |
We currently use the https://jspm.org/ API in
importmap:require
to find a CDN URL for an npm package - just like Rails. Unfortunately, this is NOT as robust as we had thought. For me, it's broken. 3 big issues:A) Not Combined
Some packages are not packed/combined. Example: chart.js/auto imports other packages and results in 3 requests instead of 1. Not TERRIBLE... so here IS a terrible example: @popperjs/core (needed by
bootstrap
) results in nearly 50 requests ❗B) Downloading Broken
For some packages, downloading simply doesn't work - rails/importmap-rails#65.
@popperjs/core
is another good example. Many of its imports have the formimport"./utils/getOppositeVariationPlacement.js
. If we download the main file, it looks locally for thatutils/
file, which won't be there. @chart.js/auto is another example.C) process.env.NODE_ENV included
Some packages (yes, again
@popperjs/core
is a great example!) containprocess.env.NODE_ENV
inside - rails/importmap-rails#65 (comment)I believe that some package advertise an "esm" package... but just don't do a good job of creating it... or create it without the browser context in mind (or at least in a way that's inconvenient for downloading).
jsDelivr to the rescue
THANKFULLY, jsDelivr seems to have a fantastic API/hosting that is almost exactly what we want: https://www.jsdelivr.com/?docs=esm
They deliver fully "packaged" modules, where the only import is for peer dependencies - e.g. https://cdn.jsdelivr.net/npm/bootstrap@5.2.3/+esm
There IS still an issue when downloading. "Peer imports" are relative -e.g.
import*as t from"/npm/@popperjs/core@2.11.7/+esm";
However, these imports follow a VERY strict pattern. So, when--download
is passed, we parse these, download the peer dependency and update the import contents to@popperjs/core
, which works with the importmap. It's not ideal that we need to do that, but it's straightforward and works great.Sorry again for this late PR - I had assumed that jspm was robust because Rails is using it. It turns out it's robust... unless you hit a "bad" package, then it's terrible. And they're not that rare: on ux.symfony.com, I have hit several.
Thanks!