webcomponents / polyfills

Web Components Polyfills
BSD 3-Clause "New" or "Revised" License
1.13k stars 165 forks source link

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

Closed justinfagnani closed 2 weeks ago

justinfagnani commented 10 months ago

Fixes https://github.com/webcomponents/polyfills/issues/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.

bicknellr commented 10 months ago

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:

https://github.com/webcomponents/polyfills/blob/f91938fd61821f4dd53179fc32cba76fdff1c5f5/packages/tests/custom-elements/html/Document/createElement.test.html#L6-L7

justinfagnani commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

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 commented 6 months ago

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

justinfagnani commented 2 weeks ago

Completed in https://github.com/webcomponents/polyfills/pull/581