8000 [AssetMapper] Give better error message if `importmap_polyfill` is set to `true` · Issue #53725 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[AssetMapper] Give better error message if importmap_polyfill is set to true #53725

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
Closed
ThomasLandauer opened this issue Feb 1, 2024 · 7 comments
Closed

[AssetMapper] Give better error message if importmap_polyfill is set to true #53725

ThomasLandauer opened this issue Feb 1, 2024 · 7 comments
Labels
AssetMapper Bug Status: Needs Review

Comments

@ThomasLandauer
Copy link
Contributor

Symfony version(s) affected

7.0.2

Description

When setting the importmap_polyfill setting to true (instead of an entry point's name), the resulting error message reads:

An exception has been thrown during the rendering of a template ("The JavaScript module polyfill was not found in your import map. Either disable the polyfill or run "php bin/console importmap:require "1"" to install it.").

Now, when actually doing php bin/console importmap:require 1, there is indeed something added called 1.

It would be nice if the error message could be improved for this case.

How to reproduce

See above.

Possible Solution

If you think that it's not worth it, just close here :-)
The importmap_polyfill setting is properly documented at https://symfony.com/doc/current/frontend/asset_mapper.html#framework-asset-mapper-importmap-polyfill

Additional Context

No response
@smnandre
Copy link
Member
smnandre commented Feb 1, 2024

Now, when actually doing php bin/console importmap:require 1, there is indeed something added called 1.

There are some packages resolved on jsdelivr / npm with the name "1" or "2" ...

So nothing to do on this side...

Concenring the importmap_polyfill, it relates to your other issue i guess ?

@ThomasLandauer
Copy link
Contributor Author
ThomasLandauer commented Feb 1, 2024 •
edited
Loading

My scenario was: I copied the entire config/packages/asset_mapper.php file from another project. But this time I wanted to enable the polyfill, so I figured to change the $assetMapperConfig->importmapPolyfill(false); line to true, which apparently gets translated to 1...

I thought this might be a trap into which other lazy developers could run too. So if there's an easy fix, you could maybe forbid true as value (or so...)
On the other hand, it's documented properly, so if you don't see any easy fix, just close here.

@javiereguiluz
Copy link
Member

I agree this could be confusing. We could make ->importmapPolyfill(true) throw a better error message ... or allow ->importmapPolyfill(true) and make it use the default shim.

@smnandre
Copy link
Member
smnandre commented Feb 3, 2024

To treat true / null / "not set" as default value, while still displaying the default in "sf debug:config framework asset_mapper" ..

i will need a hand.. i can't find a way to do this without repeating the value in the config.

(poke @weaverryan ? 👼 )

If we want to throw an error, it's easier.

@ThomasLandauer
Copy link
Contributor Author

IMO: Then just throw the error! Anything that prevents people from actually doing php bin/console importmap:require 1 is good enough :-)

@smnandre
Copy link
Member
smnandre commented Feb 3, 2024

I wont preventing anyone from running php bin/console importmap:require 1 manually.

What i can do is throw an error if someone set "true" as value for importmap_polyfill.

Is that ok for you ?

@ThomasLandauer
Copy link
Contributor Author

Sure!

@smnandre smnandre mentioned this issue Feb 3, 2024
Merged
@fabpot fabpot closed this as completed in 2fcd5be Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
AssetMapper Bug Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@javiereguiluz @ThomasLandauer @smnandre @carsonbot
0