Skip to content
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

[scoped-custom-element-registry] Unconditionally install the polyfill #562

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

justinfagnani
Copy link
Collaborator

Fixes #549

This is the simplest way to do it - just remove the dangerous feature check. I would maybe want to export a function to install the polyfill, but this is directly built as a classic script that can't have imports, so that would require a different approach. I'm not sure how optional installation is handled in other polyfills - we can add that later. The import part is not getting more deploys in the wild with the conditional installation.

Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

This helps, but if this is misaligned with the spec then it will break code that assumes spec compliance.

Much better to make it a ponyfill.

@bicknellr
Copy link
Collaborator

If this polyfill is outdated / wrong to the point that optionally installing it is dangerous given new knowledge of where the spec is heading, this seems fine. However, if this polyfill is going also to be updated to match the new spec behavior, then I think the fixed version should again install depending on if the feature already exists, but with an added flag to allow it to be forcibly enabled like the other polyfills in this repo so that we don't end up in this situation again. For example:

(window.customElements =
window.customElements || {}).forcePolyfill = true;

@justinfagnani
Copy link
Collaborator Author

@rictic

This helps, but if this is misaligned with the spec then it will break code that assumes spec compliance.

There is no spec and so no such code that can assume compliance. Current code will work with this polyfill or just not exist. This change ensures that current code does not break if a browser adds ShadowRoot.prototype.createElement but doesn't match the behavior of this polyfill.

I think we mush absolutely do this much to make sure that any page that's already using the polyfill can update and deploy this with no other changes. We may also want to change this to a ponyfill, but that is another effort that won't necessarily fix the problem here.

@justinfagnani
Copy link
Collaborator Author

@bicknellr I don't think we can adopt that strategy yet because there is no way to say that this polyfill matches any spec without there being a spec.

If we add any conditionality back here I think it should be a way override the default of installing, ie the inverse of forcePolyfill, like window.CustomElementRegistry.enableScopedRegistryPolyfillFeatureDetection = true.

@rictic
Copy link
Contributor

rictic commented Aug 30, 2023

There is no spec and so no such code that can assume compliance. Current code will work with this polyfill or just not exist. This change ensures that current code does not break if a browser adds ShadowRoot.prototype.createElement but doesn't match the behavior of this polyfill.

I think we mush absolutely do this much to make sure that any page that's already using the polyfill can update and deploy this with no other changes. We may also want to change this to a ponyfill, but that is another effort that won't necessarily fix the problem here.

There will be a spec though.

I agree that this change is good, I'm only arguing that it's insufficient, and that we should aim (in follow up work) to make it a ponyfill. That, combined with this, should resolve most future compat worries

@regevbr
Copy link

regevbr commented Dec 26, 2023

I found another case why this is needed - if you enable chrome://flags/#enable-experimental-web-platform-features the polyfill will not be installed, but importNode feature is not implemented by the chrome flag and as such there is no way to add it and things break

artem-sedykh/mini-climate-card/issues/137

Base automatically changed from scoped-element-typescript to master May 23, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants