Skip to content

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

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

Open
wants to merge 8 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

@talentlessguy
Copy link
Author

@manast do I set the engines.node field to Node 16? or there is a better approach

@manast
Copy link
Contributor

manast commented Mar 3, 2025

I think the engines.node is the most established way of doing this.

@talentlessguy
Copy link
Author

@manast just added engines.node

@talentlessguy
Copy link
Author

Is there anything to be done on this PR besides conflict merging?

@manast
Copy link
Contributor

manast commented May 3, 2025

Is there anything to be done on this PR besides conflict merging?

No, but it is a breaking change so we must wait until next major release.

@manast manast requested a review from Copilot May 3, 2025 07:57
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the codebase by removing the redundant AbortController polyfill and eliminating the uuid package in favor of Node’s built‐in crypto.randomUUID. Key changes include:

  • Replacing all instances of the uuid.v4() function with crypto.randomUUID() in both tests and production files.
  • Removing the AbortController polyfill from the utils file.

Reviewed Changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/* Updated uuid calls to use crypto.randomUUID in multiple test files
src/utils.ts Removed unnecessary polyfill import for AbortController
src/classes/worker.ts Replaced v4 with randomUUID for worker ID generation
src/classes/queue.ts Updated token generation to use randomUUID
src/classes/flow-producer.ts Switched uuid-based job IDs to use randomUUID
Files not reviewed (1)
  • package.json: Language not supported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants