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

Remove various polyfills #5448

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Remove various polyfills #5448

merged 3 commits into from
Feb 7, 2024

Conversation

mrxz
Copy link
Contributor

@mrxz mrxz commented Feb 6, 2024

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
Copy link
Member

dmarcos commented Feb 6, 2024

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

@WebReflection
Copy link

@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
Copy link
Contributor Author

mrxz commented Feb 7, 2024

@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
Copy link
Member

dmarcos commented Feb 7, 2024

Thanks both for the work and the feedback 👏

@dmarcos dmarcos merged commit 5d3f449 into aframevr:master Feb 7, 2024
1 check passed
@WebReflection
Copy link

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 👍

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.

None yet

3 participants