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
feat(extensions/crypto): implement subtle.digest #10796
feat(extensions/crypto): implement subtle.digest #10796
Conversation
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.
Glad to see this is being picked up again, thanks Casper!
const registeredAlgorithms = supportedAlgorithms[op]; | ||
const algorithmName = Object.keys(registeredAlgorithms) | ||
.find((key) => key.toLowerCase() == initialAlgorithm.name.toLowerCase()); |
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'm not thrilled by how roundabout and complicated this approach is. Could simply be:
// Should match op_crypto_subtle_digest() in extensions/crypto/lib.rs
function digestToId(name) {
switch (name) {
case "SHA-1": return 0;
case "SHA-256": return 1;
case "SHA-384": return 2;
case "SHA-512": return 3;
}
throw new DOMException(/* ... */);
}
name = name.toUpperCase();
const id = digestToId(name);
const normalizedAlgorithm = { name };
// etc.
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.
Initial pass was like that, now the whole normalisation thing follows the spec very literally
|
||
algorithm = webidl.converters.AlgorithmIdentifier(algorithm, { | ||
context: "Argument 1", | ||
}); |
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.
Add prefix
argument too: https://github.com/denoland/deno/blob/main/extensions/web/08_text_encoding.js#L158
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.
LGTM! Thanks for seeing this through :-)
Superceedes #9069
Co-authored-by: Yacine Hmito yacinehmito@users.noreply.github.com