[AssetMapper] Fix URL pattern when importing es-module-shims #53003
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
Looks like we had similar ideas: https://github.com/symfony/symfony/compare/6.4...broadcoder:symfony:fix_wrong_esm_loading_for_es_modules_shim_package_in_asset_mapper?diff=unified&w= I wanted to leave it here, since I also started to work on it until i realised I would need to spend quite some time setting up a proper environment, but maybe you like my idea of having a list of excludes instead of a single case special handling, but very likely your solution to pass also the $entry->type is better than mine, tho :) Thanks for taking care 🙏 |
| return self::URL_PATTERN_DIST_CSS; | ||
| } | ||
|
|
||
| if (str_ends_with($path, '.css') || ($type && ImportMapType::CSS === $type)) { |
There was a problem hiding this comment.
$type && is useless as null won't be equal to ImportMapType::CSS anyway
There was a problem hiding this comment.
Hi, @broadcoder! If there's a future need for more URL_PATTERN_DIST_CSS exclusions based on package name, I think that approach makes total sense. I wonder if this is really just a single edge case? @weaverryan or others? If so, it might make sense for me to drop this PR and for you to submit yours. I don't want to hijack your effort.
Sidenote: pretty sure I spent most of this PR trying to get that method name right, so props to thinking too alike. :)
There was a problem hiding this comment.
Thanks, @stof. I removed that null check.
9df3e00 to
9352217
Compare
src/Symfony/Component/AssetMapper/ImportMap/Resolver/JsDelivrEsmResolver.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/Resolver/JsDelivrEsmResolver.php
Outdated
Show resolved
Hide resolved
|
Thank you @hashbanged. |
|
Thank you, everyone, for the guidance on this. |
This fixes a JS console syntax error when importing the
es-module-shimspolyfills packageRequiring
es-module-shimsusing AssetMapper causes a trailingexport {t as default};in the downloaded file. The recommendation in #52783 is to be explicit about checking if the package is the shims/polyfill, and, if so, to use the CSS URL pattern when building the HTTP request for the package. This PR adds a new method for handling this case, and it centralizes the logic for determining if the URL pattern should beURL_PATTERN_DIST|URL_PATTERN_DIST_CSS