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

Dedupe source files #138

Merged
merged 1 commit into from
Nov 20, 2022
Merged

Conversation

hurrymaplelad
Copy link
Contributor

@hurrymaplelad hurrymaplelad commented Oct 23, 2022

Depends on #137

This diff tackles point 2 from #137 - lots of crypto code is duplicated.

This problem seems worth solving both to make sure the crypto functions evolve consistently, and to reduce merge and rebase pain for contributors.

See transcludeModule for the crux of this diff. We use a very simple CommonJS -> browser transform to share some pure functions between the node CLI and browser.

The extra separation between codec.js and impl-cryptojs.js paves the way for easily switching between CryptoJS and WebCrypto.

@robinmoisson
Copy link
Owner

This is looking really interesting - I'll need more time to look into it in detail. Again, thank you for the great PRs and comments!

@hurrymaplelad
Copy link
Contributor Author

FYI @robinmoisson - rebased onto the merged version of #137. Should be much less noisy to review now!

@robinmoisson robinmoisson mentioned this pull request Nov 12, 2022
@robinmoisson
Copy link
Owner

Hey @hurrymaplelad, thanks again for your PR and proposals! Nice job managing to refactor common code between node and browser runtimes.

I think this will allow to support multiple implementations and opens the door to new features like #112 (where suddenly the same decryption logic can easily by used in CLI or in browser).

I had some comments about moving things that were hard to convey, so I created a second proposal here #141 instead. Feel free to comment there if you have any thoughts. Thank you!

@robinmoisson robinmoisson merged commit 225d7b2 into robinmoisson:main Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants