NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[AssetMapper] Fix URL pattern when importing es-module-shims#53003

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:6.4fromhashbanged:fix_52783

Dec 20, 2023

Merged

[AssetMapper] Fix URL pattern when importing es-module-shims#53003

fabpot merged 1 commit intosymfony:6.4fromhashbanged:fix_52783

Dec 20, 2023

Conversation

Copy link

Contributor

hashbanged commentedDec 12, 2023

This fixes a JS console syntax error when importing thees-module-shims polyfills package

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix#52783
License	MIT

Requiringes-module-shims using 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

carsonbot added Status: Needs Review AssetMapper Bug labels

Dec 12, 2023

carsonbot added this to the6.4 milestone

Dec 12, 2023

Copy link

carsonbot commentedDec 12, 2023

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 7.1 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

hashbanged mentioned this pull request

Dec 12, 2023

[AssetMapper] Import-Polyfill throws error in JS-Console.#52783

Closed

Copy link

broadcoder commentedDec 13, 2023

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 🙏

stof reviewed

Dec 13, 2023

View reviewed changes

src/Symfony/Component/AssetMapper/ImportMap/Resolver/JsDelivrEsmResolver.php Outdated

		returnself::URL_PATTERN_DIST_CSS;
		}

		if (str_ends_with($path,'.css') \|\| ($type && ImportMapType::CSS ===$type)) {

Copy link

Member

stofDec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

$type && is useless asnull won't be equal toImportMapType::CSS anyway

Copy link

ContributorAuthor

hashbangedDec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hi,@broadcoder! If there's a future need for moreURL_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. Idon'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. :)