FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Remove various polyfills by mrxz · Pull Request #5448 · aframevr/aframe · GitHub

Remove various polyfills - #5448

Merged
dmarcos merged 3 commits into
aframevr:masterfrom
mrxz:remove-polyfills
Feb 7, 2024
Merged

Remove various polyfills#5448
dmarcos merged 3 commits into
aframevr:masterfrom
mrxz:remove-polyfills

Conversation

mrxz commented Feb 6, 2024

Copy link
Copy Markdown
Contributor

Description:
This PR removes the polyfills that should only really impact IE11 (or other older browsers). See #5447 for context.

Changes proposed:

  • Remove @ungap/custom-elements, present, promise-polyfill and custom-event-polyfill
  • Remove String.startsWith polyfill
  • Replace object-assign ponyfill with Object.assign

dmarcos commented Feb 6, 2024

Copy link
Copy Markdown
Member

@WebReflection Do you think the we should still keep the custom elements polyfill around? I'm sure he has a more complete picture than me.

Copy link
Copy Markdown

@WebReflection Do you think the we should still keep the custom elements polyfill around? I'm sure he has a more complete picture than me.

If you don’t use builtin extends and you don’t use the argument in whenDefined resolved promise I think the time to drop that polyfill might be right but I’d rather check dependent projects as they might assume a-frame provides the polyfill and might trust those features so that it might break others.

mrxz commented Feb 7, 2024

Copy link
Copy Markdown
Contributor Author

@WebReflection Thanks, A-Frame uses neither of those. Good point regarding dependent projects. In general adding new (A-Frame) custom elements will be done through registerPrimitive which is all handled inside A-Frame. So it'd only be a potential problem for projects that use A-Frame and Custom Elements outside of A-Frame (yet depend on the polyfill provided by A-Frame).

It probably suffices to mention it in the release notes, along with instructions on including the polyfill manually for those affected by the change.

dmarcos commented Feb 7, 2024

Copy link
Copy Markdown
Member

Thanks both for the work and the feedback 👏

dmarcos merged commit 5d3f449 into aframevr:master Feb 7, 2024

Copy link
Copy Markdown

It probably suffices to mention it in the release notes, along with instructions on including the polyfill manually for those affected by the change.

Absolutely, my comment was rather about the fact I think dropping polyfills should be considered a breaking change or nothing less than a minor release (but as it could break, maybe breaking change is more appropriate, semver speaking).

If you think the risk is minimal though and this library users won't be affected, a note is likely all you need 👍

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

3 participants


Back | FazBrowse Home | New Git URL