-
Notifications
You must be signed in to change notification settings - Fork 106
feat: replace uuid.v4 with crypto.randomUUID() #1657
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
base: main
Are you sure you want to change the base?
Conversation
boolean isRequired = binding.getMember().isRequired(); | ||
String idempotencyComponent = (isIdempotencyToken && !isRequired) ? " ?? generateIdempotencyToken()" : ""; | ||
String idempotencyComponent = (isIdempotencyToken && !isRequired) ? " ?? crypto.randomUUID()" : ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is runtime agnostic file, and crypto
is not available globally the minimum version of Node.js, i.e. 18.0.0, as per my tests
$ nvm use 18.0.0
Now using node v18.0.0 (npm v8.6.0)
$ cat randomUUID.mjs
console.log(crypto.randomUUID());
$ node randomUUID.mjs
file:///Users/trivikr/workspace/test/randomUUID.mjs:1
console.log(crypto.randomUUID());
^
ReferenceError: crypto is not defined
at file:///Users/trivikr/workspace/test/randomUUID.mjs:1:13
at ModuleJob.run (node:internal/modules/esm/module_job:198:25)
at async Promise.all (index 0)
at async ESMLoader.import (node:internal/modules/esm/loader:409:24)
at async loadESM (node:internal/process/esm_loader:85:5)
at async handleMainPromise (node:internal/modules/run_main:61:12)
Node.js v18.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The crypto was added globally as follows:
- v23.0.0 | No longer experimental.
- v19.0.0 | No longer behind --experimental-global-webcrypto CLI flag.
- v17.6.0, v16.15.0 | Added in: v17.6.0, v16.15.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can replace uuid
with a utility in core though, where Node.js one will get it from crypto.
That utility can be removed once we drop support for Node.js 22.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something blocking us from simply importing crypto module the old way? randomUUID()
method in node:crypto
module was added in Node.js v15.6.0, v14.17.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code that currently uses randomUUID would no longer be "isomorphic" compatible.
We will revisit this soon, since I am in the process of rewriting that part of the SDK code.
aws/aws-sdk-js-v3#7212
replace uuid v4 with crypto.randomUUID()
execution environments without global crypto.randomUUID will need a replacement polyfill.