-
Notifications
You must be signed in to change notification settings - Fork 166
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
Conversation
There was a problem hiding this 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.
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: polyfills/packages/tests/custom-elements/html/Document/createElement.test.html Lines 6 to 7 in f91938f
|
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 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. |
@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 |
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 |
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 |
Completed in #581 |
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.