Skip to content

[Site] Load importmap polyfill conditionnaly #1381

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

Merged

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Jan 7, 2024

With importmap support now over 90% of total usage.. i think we can try something here :)

@smnandre
Copy link
Member Author

smnandre commented Jan 7, 2024

@weaverryan
Copy link
Member

@smnandre do you need anything from me on this? Like a deploy of a dev environment to test? Ultimately, I'd like to move this to a PR on Symfony :)

@weaverryan weaverryan added Status: Waiting Feedback Needs feedback from the author Feature New Feature labels Jan 29, 2024
@smnandre
Copy link
Member Author

We need something to test things with non-compatible browsers...

Cannot find it anymore on Slack, but I suggested to get a BrowserStack / similar account.

(on this website... to be honest, i'd skip polyfill entirely with overall support over 92% and this website beeing browsed almost only by tech working visitors haha)

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

The code used here is minimal and won't easily change ... so I'd say we don't need to test it in a real legacy browser.

@smnandre
Copy link
Member Author

The code used here is minimal and won't easily change ... so I'd say we don't need to test it in a real legacy browser.

But i'd like to be sure it's working ... and my first attempt did not (because HTMLScriptElement.supports was not available on old browser).

We also (and i'm almost not kidding) can merge this, test live on ux.symfony.com and wait for people experiencing bug, and then fix/built on that.

The last experiments i've done with AssetMapper showed me that the polyfill performance cost is far from minor (at least for someone so crazyfocused on performances).

So believe me when i say i'm the first one wanting to remove it for the vast majority of users that do not need it :)

@weaverryan weaverryan force-pushed the feat/load-importmap-polyfill-conditionnaly branch from 5c3aab8 to d322b5b Compare January 30, 2024 15:14
@weaverryan
Copy link
Member

Thanks! Let's deploy and try it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Waiting Feedback Needs feedback from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants