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

Add WASM Support #52

Merged
merged 1 commit into from
Feb 8, 2021
Merged

Add WASM Support #52

merged 1 commit into from
Feb 8, 2021

Conversation

theosirian
Copy link
Collaborator

@theosirian theosirian commented Feb 5, 2021

Adds WASM support to DIDKit using wasm-pack/wasm-bindgen.

Added a wasm feature on DIDKit to prevent compiling C and Java integration on the wasm-pack build.
Added DIDKitLoader for web targets to facilitate working with the WASM code on browsers.
Added a test for all current methods on DIDKit using DIDKitLoader and the WASM build.

This is a substitute for #15.

@theosirian
Copy link
Collaborator Author

@clehner the build failed because of the issue with how default-features work on workspaces. Do you have any ideas on how to work around it?

@clehner
Copy link
Contributor

clehner commented Feb 5, 2021

Looks good. @theosirian I think this might fix the issue with features: spruceid/ssi#85

Also looks like we must make the didkit crate also not depend on ssi's default-features:

diff --git a/lib/Cargo.toml b/lib/Cargo.toml
index 954ccc6..4b3a3a8 100644
--- a/lib/Cargo.toml
+++ b/lib/Cargo.toml
@@ -9 +9 @@ edition = "2018"
-ssi = { path = "../../ssi" }
+ssi = { path = "../../ssi", default-features = false }

@clehner
Copy link
Contributor

clehner commented Feb 5, 2021

Works for me:

[    ok: should get library version] index.test.js:4:13
[    ok: should generate ed25519 key] index.test.js:4:13
[    ok: should produce did] index.test.js:4:13
[    ok: should produce verificationMethod] index.test.js:4:13
[    ok: should fail if parameters are empty objects] index.test.js:4:13
[    ok: should verify issued credential] index.test.js:4:13
[    ok: should fail if parameters are empty objects] index.test.js:4:13
[    ok: should verify issued presentation]

I had to use wasm-pack build --target web instead of just wasm-pack build. Should that be noted in the readme?

I can build and run it with #55 and the following merged:

diff --git a/lib/Cargo.toml b/lib/Cargo.toml
index 341b4ab..03c543b 100644
--- a/lib/Cargo.toml
+++ b/lib/Cargo.toml
@@ -9,2 +9,3 @@ default = ["ring"]
 ring = ["ssi/ring"]
+wasm = []
 
@@ -24,5 +25 @@ lazy_static = "1.4"
 crate-type = ["lib", "staticlib", "cdylib"]
-
-[features]
-default = []
-wasm = []
diff --git a/wasm/Cargo.toml b/wasm/Cargo.toml
index 18793e3..ac0a460 100644
--- a/wasm/Cargo.toml
+++ b/wasm/Cargo.toml
@@ -18,2 +18,3 @@ chrono = { version = "0.4", features = ["wasmbind"] }
 path = "../lib"
+default-features = false
 features = ["wasm"]

I see more of what you mean about the behavior of workspace features. i.e. "Features in a workspace are unified across all roots selected on the command-line." rust-lang/cargo#8366 (comment)
It seems to mean we can't build didkit-wasm in the same CLI invocation as other crates depending on non-WASM implementations of the crypto. But by using --exclude, we can split the invocations into multiple invocations with different feature selections, like this:

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index ed1b1a3..3cbb0ae 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -53,6 +53,9 @@ jobs:
     - name: Build
-      run: cargo build --verbose --workspace
+      run: cargo build --verbose --workspace --exclude didkit-wasm
 
-    - name: Test
-      run: cargo test --verbose --workspace
+    - name: Test (non-WASM)
+      run: cargo test --verbose --workspace --exclude didkit-wasm
+
+    - name: Test (WASM)
+      run: cargo test --verbose -p didkit-wasm

Also, what about testing didkit-wasm in CI? There is a wasm-pack test command which runs tests in Node or in a browser. Some other projects are doing this:
https://github.com/search?q=wasm-pack+path%3A%2F.github%2Fworkflows&type=Code&ref=advsearch
https://rustwasm.github.io/wasm-pack/book/tutorials/npm-browser-packages/testing-your-project.html
But it looks like that is for running Rust tests in the WASM environment, not JS tests using the built WASM module. I don't see any typical way of testing JS tests from wasm-pack output... https://stackoverflow.com/questions/65710914/is-it-possible-to-test-wasm-pack-generated-webassembly-from-a-javascript-context

@theosirian
Copy link
Collaborator Author

Building correctly for me as well.

I also didn't find anything on testing from the JS side. We could try manually running wasm/loader/test/index.test.js on a headless browser, but I'm not sure how much work that would involve.

@clehner
Copy link
Contributor

clehner commented Feb 5, 2021

Reopened because accidentally auto-closed by spruceid/ssi#85 (comment).

@clehner clehner reopened this Feb 5, 2021
@clehner
Copy link
Contributor

clehner commented Feb 5, 2021

Maybe it would be possible to have tests run by wasm-pack test load and run the JS tests from Rust code.

Or we could just do the build only, and manually test it in the browser when preparing releases.

@theosirian
Copy link
Collaborator Author

Something like selenium-rs to load the webpage with the test?
Or more like instantiating a JS engine like V8 to run the code?

For now, maybe manually testing is okay.

@theosirian
Copy link
Collaborator Author

Rebased onto current main, moved the WASM package into lib/ and added targets on Makefile and Github Actions.

@theosirian theosirian force-pushed the wasm-async branch 2 times, most recently from 3f8c3aa to f51551a Compare February 8, 2021 03:34
Adds WASM support to DIDKit using `wasm-pack`.
@clehner
Copy link
Contributor

clehner commented Feb 8, 2021

Thanks @theosirian. Looks good; waiting for CI now.

@clehner clehner merged commit 09ce2c4 into main Feb 8, 2021
@clehner clehner deleted the wasm-async branch February 8, 2021 14:51
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