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

doc: mark Certificate methods as static, add missing KeyObject.from #37198

Closed
wants to merge 1 commit into from

Conversation

panva
Copy link
Member

@panva panva commented Feb 3, 2021

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Feb 3, 2021
@panva panva force-pushed the doc-crypto-missing-static-methods branch from eae8700 to 5218bbe Compare February 3, 2021 10:12
@panva panva changed the title doc: mark crypto methods as static, add missing KeyObject.from doc: mark Certificate methods as static, add missing KeyObject.from Feb 3, 2021
@panva panva force-pushed the doc-crypto-missing-static-methods branch from 5218bbe to 9396d6d Compare February 3, 2021 10:33
@panva panva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 3, 2021
@tniessen
Copy link
Member

tniessen commented Feb 5, 2021

I realize that this has landed and was released, but regarding KeyObject.from see #37218 (comment):

While we are at it, I have to say I'm not happy that #35093 added KeyObject.from. It is public yet undocumented and has a super generic name yet serves a super specific purpose. A purpose that should rarely be necessary if WebCrypto actually accomplished its goals of feature completeness and portability.

Not really related to the changes in this PR though, at least the function won't be undocumented anymore.

@panva
Copy link
Member Author

panva commented Feb 5, 2021

@tniessen would you like to introduce a specifically named method instead and mark this one deprecated? Either way i think this doc update should land.

@tniessen
Copy link
Member

tniessen commented Feb 5, 2021

would you like to introduce a specifically named method instead and mark this one deprecated?

That depends. It works and there is no ambiguity when it comes to the input. We can reuse the same function with different signatures for other inputs.

It depends on what other input forms we want to allow in the future. What about JWK, for example? If we allow KeyObject.from(jwkObject), then we suddenly create the chance of having ambiguous inputs because JWK is a plain object and not an instance of a class that is specific to Node.js.

In the other PR (#37218), adding KeyObject.prototype.toCryptoKey(webCryptoAlgorithm, usages, extractable) was suggested. If that's going to happen, then renaming KeyObject.from to KeyObject.fromCryptoKey might make sense. I've taken the liberty of opening #37240.

If we never want to allow anything but uniquely identifiable types, such as CryptoKey, as inputs to KeyObject.from, then we might as well leave it at that. However, I'm afraid that at some point, someone will try to fit the generic API name to other inputs.

@tniessen
Copy link
Member

tniessen commented Feb 5, 2021

(Approving under the assumption that #37240 will update the docs accordingly.)

@panva
Copy link
Member Author

panva commented Feb 5, 2021

Landed in e8286bb

panva added a commit that referenced this pull request Feb 5, 2021
PR-URL: #37198
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@panva panva closed this Feb 5, 2021
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
PR-URL: #37198
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
This was referenced Feb 16, 2021
@panva panva deleted the doc-crypto-missing-static-methods branch October 13, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants