Skip to content

test(crypto): test ReadableStream in digest #2766

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 18, 2022
Merged

test(crypto): test ReadableStream in digest #2766

merged 4 commits into from
Oct 18, 2022

Conversation

@iuioiua
Copy link
Contributor

@iuioiua iuioiua commented Oct 10, 2022

I'm neither an expert on the Stream nor Web Crypto APIs, so please let me know of any concerns. Testing is basic for now but can be expanded if needed.

Part of #2733.

@kt3k
Copy link
Member

kt3k commented Oct 17, 2022

(Sorry for the delay in review. We were busy last week)

Oh, we already support AsyncIterable<BufferSource> source.. (That makes readable stream input automatically working..) I wasn't fully aware of it..

@littledivy
Copy link
Member

littledivy commented Oct 17, 2022

Maybe we should wait for https://github.com/wintercg/proposal-webcrypto-streams?

@kt3k
Copy link
Member

kt3k commented Oct 17, 2022

std/crypto seems deviating from web crypto from the beginning. ref. #1025

And because it supports AsyncIterable<BufferSource> as input, the readable stream seems already supported

@kt3k
Copy link
Member

kt3k commented Oct 17, 2022

I think this change is unnecessary and my previous comment (std/crypto doesn't support streaming) was wrong. I think we can just deprecate & remove std/hash now.

@iuioiua
Copy link
Contributor Author

iuioiua commented Oct 17, 2022

I think this change is unnecessary and my previous comment (std/crypto doesn't support streaming) was wrong. I think we can just deprecate & remove std/hash now.

I'll tweak this PR so it just provides tests for digesting streams instead.

@iuioiua iuioiua changed the title feat(crypto): support ReadableStream in digest feat(crypto): test ReadableStream in digest Oct 17, 2022
@iuioiua iuioiua mentioned this pull request Oct 18, 2022
Merged
kt3k
kt3k approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kt3k kt3k changed the title feat(crypto): test ReadableStream in digest test(crypto): test ReadableStream in digest Oct 18, 2022
@kt3k kt3k merged commit f7e6609 into denoland:main Oct 18, 2022
@iuioiua iuioiua deleted the crypto-streams branch October 18, 2022 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kt3k kt3k kt3k approved these changes

@bartlomieju bartlomieju Awaiting requested review from bartlomieju

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iuioiua @kt3k @littledivy