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

Inject ElementInternals polyfill where needed #20818

Merged
merged 1 commit into from
May 23, 2024

Conversation

steverep
Copy link
Member

Proposed change

Inject the ElementInternals polyfill using the Babel plugin and remove manual imports.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@github-actions github-actions bot added the Build Related to building the code label May 17, 2024
@steverep steverep marked this pull request as ready for review May 17, 2024 22:08
Copy link
Contributor

@silamon silamon left a comment

Choose a reason for hiding this comment

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

How to test these? Do you force the browser to disable the elements internal api, so the polyfill get loaded?

@steverep
Copy link
Member Author

How to test these? Do you force the browser to disable the elements internal api, so the polyfill get loaded?

Yes. For this I'd put window.ElementInternals = undefined in index.html. I also examine exactly what modules get the injection after running Webpack.

@silamon
Copy link
Contributor

silamon commented May 23, 2024

Yeah, code-wise I checked on caniuse. Thanks for clarifying.

I've did another test with window.ElementInternals set to undefined, but to me it looks like it never tries to load the polyfill if it needs to. The polyfill was introduced for material/web and codewise it's doing (this as HtmlElement).attachInternals() so actually it's using HTMLElement.prototype.attachInternals which looking further is native code. I would expect that to be (minified) javascript code from the polyfill package.

Importing import { isElementInternalsSupported } from "element-internals-polyfill/dist/element-internals"; and returning the boolean result says false where I would expect it to be polyfilled already.

Edit: Let me try to force the legacy build as well.

@steverep
Copy link
Member Author

I've did another test with window.ElementInternals set to undefined, but to me it looks like it never tries to load the polyfill if it needs to. The polyfill was introduced for material/web and codewise it's doing (this as HtmlElement).attachInternals() so actually it's using HTMLElement.prototype.attachInternals which looking further is native code. I would expect that to be (minified) javascript code from the polyfill package.

Importing import { isElementInternalsSupported } from "element-internals-polyfill/dist/element-internals"; and returning the boolean result says false where I would expect it to be polyfilled already.

The attachInternals method just returns an instance of the ElementInternals class for the element, so the polyfill just checks for the existence of the class and several of its methods. You could delete the method from the element prototype and get the same result I think.

Edit: Let me try to force the legacy build as well.

Injections are identical given the age of the modern targets. Only difference is transpiling the polyfill, which we are already doing.

@silamon
Copy link
Contributor

silamon commented May 23, 2024

I've got it to work correctly as I would expect on the legacy build (hence the approval). With the last paragraph, do you mean that it should also inject the polyfill on the modern build? If so, when?

@steverep
Copy link
Member Author

Injections are exactly the same on the modern build.

@steverep steverep merged commit e0062cf into dev May 23, 2024
14 checks passed
@steverep steverep deleted the inject-element-internals-polyfill branch May 23, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Related to building the code cla-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants