Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 initial support for wasm target #92
base: main
Are you sure you want to change the base?
add initial support for wasm target #92
Changes from 5 commits
dbb25c0
f6b499c
4738b66
446d53a
883c735
1a6e951
391b3f2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this also contain the default "lib" crate type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. In my tests, it wasn't necessary. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should contain the default "lib" as well. I will need to read up on this setting more before merging this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmeringdal setting the Cargo.toml file as:
Once I run cargo build it's showing the below error:
How do you recommend to fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it build with just "lib"? Why was "lib" replaced with ["cdylib", "rlib"]. I am not that familiar with how WASM internals work and don't want to merge anything I don't fully understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmeringdal @antoinepairet
Assuming in our context the below command as Rust Build:
And the below command as Wasm Build:
wasm-pack build --release --target web --features "wasm"
We can analyze the different outputs:
1.a) the Rust Build works.
1.b) the Wasm Build fails:
2.a) the Rust Build and Wasm Build command fail and return the same output:
3.a) the Rust Build and Wasm Build command build successfully (the current PR is using this config).
I've found two docs that does not answer this type of problem:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context @dgmachado . I think the wasm build is fine, my main concern is the implications of the Rust build and how it might affect applications relying on this library. Before resolving this discussion I will have to do a deeper dive into WASM myself to understand what is going on and why this is required. Sorry for holding up this PR, but I can't merge anything that I don't fully understand as I have to maintain this.