-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
doc: deprecate crypto.getRandomValues
alias
#41779
Conversation
What's the reason for deprecating this? It just says "non-standard behaviors"... Not clear what those are. Reading through the other issue, I'd argue that the illegal this error is not really sufficient justification. |
This alias is basically useless if we enforce const { getRandomValues, webcrypto } = require('node:crypto')
getRandomValues.call(webcrypto, new Uint8Array) What value is there in keeping an alias like that? |
I agree, the doc deprecate description could probably use more detail so end users can have a better idea if additional changes are needed in their code to make things "standard." |
I'd say that it's just a bug if it can't be called directly without passing in the Crypto object as this. Let's fix that bug and leave this alone otherwise. I see no reason to deprecate |
Web browsers and Deno enforce that |
The alias doesn't have to have the same limitations as the spec-compliant one |
I discover this limitation because I worked on a integrating on the web a Node.js library that was using WebCrypto, and got really confused by the |
I don't think we should have a more relax implementation. I think we can make |
By the way, there is the same issue with |
Maybe I'm confused on the use case here. My assumption is folks who are using
|
Also, |
It's closer to From my PoV, exposing
This wouldn't help people doing |
If instead, we add a semver-minor change to |
OK, but let's be clear in the docs this is NOT an alias to the webcrypto one then. |
I have nothing against specifying in the docs that contrary to It's also worth asking in the spec repo why |
I don't know what to tell you, I've been affected by it (and therefore I care), and I don't think I'm the only one trying to use WebCrypto to write web-compatible code. Or if you mean the alias specifically, well it's blocking the other PR so...
Definitely, I also think this condition is silly. I won't do it, but if someone feels like they can to open an issue to see if the rest of the ecosystem can aligns with the more relaxed behavior, that'd be nice. |
I do mean the alias specifically. |
This seems like a larger problem to me. I didn't approve the aliases in the first place because of somewhat related concerns, but we have a serious problem if we start landing and releasing APIs just to deprecate them in the very next release. |
I don't see the problem as long as we follow semver rules, any API is considered stable until it is not. Although maybe this shows that we should have a policy that any new API should land as experimental first so we don't have to go through a whole deprecation cycle if problems arise when it's released. |
In this case, we could argue that being an alias of an experimental method |
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.
I am still -1 on deprecating this. The reasons for wanting to deprecate this are not at all clear.
Is #41938 going to be unflagged in a major release? It breaks the REPL. If it isn't, and |
Does it break it in the sense that the REPL doesn't work or that it's a breaking (semver-major) change? |
I'd say it's a semver-major-MAJOR-major change. It breaks every script that uses crypto through |
I didn't think about |
I don't think we have any data on that. It wouldn't surprise me to see something like this: # Figure out what OpenSSL version the installed node version is using.
node_ossl=$(node -p 'process.versions.openssl')
# Figure out what ciphers the installed node version supports.
node_ciphers=$(node -p 'crypto.getCiphers().join("\n")') |
Examples:
It also breaks scripts that uses crypto through |
No, really the key point was to align somewhat with the global web crypto so now that we're moving forward with making that global, I can drop my objection here |
I've opened #42083 to discuss make Web Crypto available on the global scope by default, except for |
I'm no longer convinced a deprecation is necessary at this time, this can be reopened at a later time if necessary. |
Refs: nodejs#41779 Refs: nodejs#41760 PR-URL: nodejs#41782 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Refs: #41760