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

use wasm-bindgen's fs.readFileSync loader in node #17

Merged
merged 6 commits into from
Sep 29, 2022

Conversation

jbr
Copy link
Contributor

@jbr jbr commented Sep 26, 2022

This was the end of a long investigation trying to address an issue with the wasm loading from within ts-mocha (use without a compile step). With the included change, we use the wasm-bindgen fs.readFileSync loader, but leave the browser build using the base64-inline loader. I believe this change also resolves the remaining outstanding issues with divviup/divviup-ts#34, allowing a new hpke-dispatch version to be published and used from within divviup-ts through npm.

@jbr jbr requested a review from a team as a code owner September 26, 2022 21:01
Copy link
Collaborator

@divergentdave divergentdave left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, though I'm not too experienced with wasm-bindgen/wasm-pack. Can we get rid of pkg/node/index.ts at this point?

@jbr
Copy link
Contributor Author

jbr commented Sep 27, 2022

Yes, I think we can get rid of index.ts, although I wasn't sure if there might still be some use cases for it, as it can be required as import * as hpke from "hpke/pkg/node". Probably best to remove and re-add if we find a need. Unfortunately the wasm packaging state of the art seems to be more tailored towards applications than libraries that hide the use of wasm as an implementation detail.

@jbr
Copy link
Contributor Author

jbr commented Sep 27, 2022

I'm not entirely sure if this is a breaking change or if it is a patch-level bugfix. As there are likely not a lot of users of this package and it seems like this "works as initially intended," I'm inclined to release this as 03.1 instead of 0.4.0, but am not at all sure of that

@jbr jbr mentioned this pull request Sep 29, 2022
@@ -1,7 +1,7 @@
{
"name": "hpke",
"description": "hybrid public key encryption",
"version": "0.3.0",
"version": "0.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the version in Cargo.toml need to be bumped in sync with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous to #18 there were no changes in the rust interface so it seemed strange to bump the version (and there's no reason the two versions need to be kept in sync) but now there is cause for a minor version bump in both

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I think we should bump the crate to 0.4.0 in this PR, and once we merge it, I'll release 0.4.0 to crates.io.

Copy link
Contributor Author

@jbr jbr Sep 29, 2022

Choose a reason for hiding this comment

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

@tgeoghegan tgeoghegan merged commit 3495002 into main Sep 29, 2022
@tgeoghegan tgeoghegan deleted the fix-loader-for-testing branch September 29, 2022 21:12
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.

3 participants