8000 [AssetMapper] Put importmap in polyfill so it can be hosted locally easily by weaverryan · Pull Request #51828 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[AssetMapper] Put importmap in polyfill so it can be hosted locally easily #51828

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 16, 2023
Merged

[AssetMapper] Put importmap in polyfill so it can be hosted locally easily #51828

merged 1 commit into from
Oct 16, 2023

Conversation

@weaverryan
Copy link
Member
@weaverryan weaverryan commented Oct 3, 2023
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #51302
License MIT

Note: Built on top of #51786

In #51302, it was asked to allow the polyfill to be an asset mapper path. We could do that, but it would still require the user to "manage" this vendor file locally - i.e. commit it into their repository. So this PR goes a bit further and requires your polyfill to be an item in importmap.php. That's a bit odd, but because of #51786, it allows the JS package to be downloaded like any other JS package. The item is actually removed from the importmap before it's finally dumped. So the small oddity of having this item in importmap.php makes a really nice DX.

Cheers!

@carsonbot carsonbot added this to the 6.4 milestone Oct 3, 2023
@weaverryan weaverryan force-pushed the asset-mapper-importmap-in-polyfill branch from 9e7e935 to 311a2fc Compare October 3, 2023 16:04
smnandre
smnandre reviewed Oct 4, 2023
View reviewed changes
src/Symfony/Component/AssetMapper/Command/ImportMapInstallCommand.php
public function __construct(
private readonly ImportMapManager $importMapManager,
private readonly RemotePackageDownloader $packageDownloader,
private readonly string $projectDir,
Copy link
Member
@smnandre smnandre Oct 4, 2023

Choose a reason for hiding this comment

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

Is this necessary "just" to render a success message ?

Copy link
Member Author
@weaverryan weaverryan Oct 4, 2023

Choose a reason for hiding this comment

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

It is... which is kind of lame, but otherwise, I'll be rendering the whole "Downloaded assets into /foo/bar/baz/my-project/assets/vendor"

@weaverryan weaverryan added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature f 8000 reeze" label Oct 6, 2023
@weaverryan weaverryan force-pushed the asset-mapper-importmap-in-polyfill branch 3 times, most recently from 5174c83 to 0cc5f9c Compare October 6, 2023 17:22
@weaverryan
Copy link
Member Author
weaverryan commented Oct 10, 2023 •
edited
Loading

I think this is needed, but one negative I thought of. We will likely need to start the importmap.php recipe with the polyfill inside of it. And since vendor files are all now always downloaded, the user will need to run importmap:install before the page will run.

We could... just for the polyfill, if not installed, revert to the CDN URL instead? Or just not render it? It seems iffy. However, missing the polyfill locally would not be an issue (we all have modern browsers). And it is HIGHLY unlikely that someone deploys with NO other vendor libraries installed in their app. And so, they would see 404 errors anyway / their importmap:compile command would explode due forgetting importmap:install.

UPDATE

All good now - here is the flow:

A) If the polyfill is set to es-module-shims (which is the default in FrameworkBundle) BUT it is not in your importmap, then it outputs the CDN URL. This will be the default behavior.

B) If the user chooses to add es-module-shims to their importmap (importmap:require es-module-shims) the system will instantly start pointing to that local file.

So the default behavior is basically identical to now, but with the option to add es-module-shims to the importmap.php to serve a local file. The user can, of course, still entirely disable the importmap and avoid it entirely, or do whatever they want manually.

@weaverryan weaverryan force-pushed the asset-mapper-importmap-in-polyfill branch 2 times, most recently from 0fd142f to 397a816 Compare October 11, 2023 13:13
@weaverryan weaverryan force-pushed the asset-mapper-importmap-in-polyfill branch from 397a816 to cb02e68 Compare October 13, 2023 17:29
@fabpot fabpot force-pushed the asset-mapper-importmap-in-polyfill branch from cb02e68 to d2014eb Compare October 16, 2023 20:21
@fabpot
Copy link
Member
fabpot commented Oct 16, 2023

Thank you @weaverryan.

@fabpot fabpot mentioned this pull request Oct 16, 2023
Closed
@fabpot fabpot merged commit 7cf6f77 into symfony:6.4 Oct 16, 2023
@weaverryan weaverryan deleted the asset-mapper-importmap-in-polyfill branch October 16, 2023 20:39
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot approved these changes

+1 more reviewer

@smnandre smnandre smnandre left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AssetMapper Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

[AssetMapper] Allow importmap_polyfill to be an AssetMapper path

4 participants

@weaverryan @fabpot @smnandre @carsonbot
0