polyfills icon indicating copy to clipboard operation
polyfills copied to clipboard

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

Open justinfagnani opened this issue 2 years ago • 5 comments

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.

justinfagnani avatar Aug 23 '23 04:08 justinfagnani

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

bicknellr avatar Aug 23 '23 20:08 bicknellr

@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 avatar Aug 23 '23 22:08 justinfagnani

@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.

justinfagnani avatar Aug 23 '23 22:08 justinfagnani

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

rictic avatar Aug 30 '23 19:08 rictic

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

regevbr avatar Dec 26 '23 11:12 regevbr

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

justinfagnani avatar Jun 20 '24 17:06 justinfagnani