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 WASI support for gloo-history. #405

Merged
merged 3 commits into from Nov 30, 2023
Merged

Conversation

langyo
Copy link
Contributor

@langyo langyo commented Nov 20, 2023

I've been trying to use yew to render the page into the static HTML string on WASI. However, gloo-history cannot distinguish the browser WASM target (wasm32-unknown-unknown with wasm-bindgen) and WASI target (wasm32-wasi), and it would choose wrong functions for wasm32-*.

I have fixed it by using target_os, and it would fix the problem.

Copy link
Collaborator

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

Can you make it use cfg(target_os = "wasi") instead of a feature
image

That should make the wasi feature unnecessary and handle the compilation accordingly. Other than that, it looks good.

It would be great if you could also update CI to run tests on WASI target (using wasmtime) though

@langyo
Copy link
Contributor Author

langyo commented Nov 30, 2023

Can you make it use cfg(target_os = "wasi") instead of a feature image

That should make the wasi feature unnecessary and handle the compilation accordingly. Other than that, it looks good.

It would be great if you could also update CI to run tests on WASI target (using wasmtime) though

It's so new that when I looked through the documentation, it didn't even mention that the value of target_os can be wasi...

I tested it on the latest stable compiler and it works. I'll push the modified version later.

Copy link
Collaborator

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

Thanks! Perhaps this is a good thing to update in the Rust documentation as well

@hamza1311 hamza1311 merged commit 7516f26 into rustwasm:master Nov 30, 2023
17 checks passed
hamza1311 added a commit that referenced this pull request Dec 6, 2023
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.

None yet

2 participants