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.

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

refactor: deno_crypto op crate #7956

Merged
merged 19 commits into from
Nov 13, 2020
Merged

refactor: deno_crypto op crate #7956

merged 19 commits into from
Nov 13, 2020

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented Oct 13, 2020 •
edited
Loading

Moves crypto to its own op_crate

@littledivy littledivy changed the title refract(cli/ops): random.rs => op_crates/crypto refractor(cli/ops): random.rs => op_crates/crypto Oct 13, 2020
ry
ry reviewed Oct 13, 2020
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

@littledivy Thanks for this patch - I think this looks good and is on the right track.

I'm wondering why you're working on this? Do you need this yourself - or just thought it'd be a good clean up?

@littledivy
Copy link
Member Author

@ry Saw it on the the Roadmap #7915, thought I'd help :)

finish op crates refactor - tests, remaining web APIs in crates

Crypto is very likely to expand in the near future so just a step towards keeping it clean for contributions.

@littledivy
Copy link
Member Author

Would you mind patches for moving most of the ops? – I didn't attempt to refract all of them in this PR as I wanted to know the core team's opinion.

@bartlomieju
Copy link
Member

Would you mind patches for moving most of the ops? – I didn't attempt to refract all of them in this PR as I wanted to know the core team's opinion.

Thanks @littledivy but we should hold off on moving more crates - it's not exactly clear yet how we should handle reexports and where to draw crates boundaries; additionally remaining JS runtime code is quite dependent on some code that is specific to cli

@littledivy littledivy marked this pull request as ready for review October 13, 2020 15:43
@littledivy
Copy link
Member Author

Should be ready to land 👍

bartlomieju
bartlomieju requested changes Oct 14, 2020
View reviewed changes
op_crates/crypto/Cargo.toml Outdated Show resolved Hide resolved
op_crates/crypto/Cargo.toml Show resolved Hide resolved
@littledivy
Copy link
Member Author

merged master.

@bartlomieju bartlomieju self-requested a review October 19, 2020 18:46
@bartlomieju bartlomieju changed the title refractor(cli/ops): random.rs => op_crates/crypto refactor: deno_crypto op crate Oct 19, 2020
@bartlomieju
Copy link
Member

bartlomieju commented Nov 9, 2020 •
edited
Loading

@littledivy please rebase, I'm still interested in landing this PR

@littledivy
Copy link
Member Author

@bartlomieju Updated. 👍 (sorry for the delay)

bartlomieju
bartlomieju approved these changes Nov 13, 2020
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @littledivy, hopefully this refactor will kick-off webcrypto APIs

@bartlomieju bartlomieju merged commit d5661f6 into denoland:master Nov 13, 2020
jannes pushed a commit to jannes/deno that referenced this pull request Dec 1, 2020
@littledivy @jannes
refactor: deno_crypto op crate (denoland#7956)
ae8cc4e 
This commit factors out "deno_crypto" op crate.

"rand" crate dependency was consequently moved to 
"deno_crypto" crate and reexported.
@littledivy littledivy deleted the refract/crypto-crate branch February 27, 2021 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry ry ry left review comments

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@littledivy @bartlomieju @ry