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

refactor: remove redundant AC polyfill and use built-in UUID generator #3067

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

talentlessguy
Copy link

Why

  1. AbortController has been available since Node 14.17, there is no longer a need to polyfill it. Node 12 has been EOL for quite some time and is considered dangerous to use. There is no longer a need for uuid as a package, since Node 16.7.0 the crypto module provides a safe generator for UUIDs.
  2. Smaller supply chain, avoidance of downloading and loading unnecessary polyfills.

How

Removed both of the packages and replaced them with native functionality.

Additional Notes (Optional)

Tests pass locally on Node 18 / 20 / 22. Can't test on Node 16 because corepack is not compatible with it.

@manast
Copy link
Contributor

manast commented Feb 10, 2025

Thanks. Always great to get rid of dependencies!

@talentlessguy
Copy link
Author

oops, forgot to replace uuid in tests, gimme a moment

@manast
Copy link
Contributor

manast commented Feb 10, 2025

it seems like there are still some references to the uuid module. Btw, we will need to decide if this can be merged in current major version or if we will need to make a breaking release as there are potential users out there using lower than 16 in production.

@talentlessguy
Copy link
Author

talentlessguy commented Feb 10, 2025

it seems like there are still some references to the uuid module. Btw, we will need to decide if this can be merged in current major version or if we will need to make a breaking release as there are potential users out there using lower than 16 in production.

I'd say a breaking change would be better, as well as settings engines.node to >=16.7

or in case you'd like to drop all EOL, >=18

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.

2 participants