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 example for CI. #411

Merged
merged 8 commits into from
Dec 6, 2023
Merged

Conversation

langyo
Copy link
Contributor

@langyo langyo commented Nov 30, 2023

#405

It's ready to review now.

@langyo langyo marked this pull request as ready for review December 5, 2023 05:25
@langyo
Copy link
Contributor Author

langyo commented Dec 5, 2023

cc @hamza1311 😉

Maybe it's time to release a patch version in cargo that supports yew's WASI related compilation targets.

Related to yewstack/yew#3534

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 for taking the time to do this

@hamza1311 hamza1311 merged commit 399dabb into rustwasm:master Dec 6, 2023
20 checks passed
hamza1311 added a commit that referenced this pull request Dec 6, 2023
@hamza1311
Copy link
Collaborator

hamza1311 commented Dec 7, 2023

Released in gloo-net 0.5 🎉

@hamza1311
Copy link
Collaborator

@langyo gloo-net 0.5 has been released. Can you update this PR?

@langyo
Copy link
Contributor Author

langyo commented Dec 7, 2023

@langyo gloo-net 0.5 has been released. Can you update this PR?

Sure

#419

@@ -13,8 +15,10 @@ use crate::{error::HistoryResult, query::ToQuery};
#[derive(Clone, PartialEq, Debug)]
pub enum AnyHistory {
/// A Browser History.
#[cfg(not(target_os = "wasi"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the effects of enabling BrowserHistory and MemoryHistory for wasi?

I think wasi should have BrowserHistory and MemoryHistory enabled like other targets.
This helps to avoid additional feature flags for downstream crates and will lint the code like other targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are the effects of enabling BrowserHistory and MemoryHistory for wasi?

I think wasi should have BrowserHistory and MemoryHistory enabled like other targets. This helps to avoid additional feature flags for downstream crates and will lint the code like other targets.

If don't, BrowserHistory is directly dependent on wasm-bindgen. It will cause additional symbols like __wbindgen_placeholder__::xxx to appear in the compiled result WASM, which is invalid in WASI.

These changes that I've made to some other repositories recently were also made to fix this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think WASI has been disabled in rustwasm/wasm-bindgen#3233?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think WASI has been disabled in rustwasm/wasm-bindgen#3233?

It seems like it will still try to import symbols if you actively reference it. This is a bit different from the situation where wasm-bindgen passively imports utility functions related to heap allocation.

Personally, I feel that it is not a bad thing to proactively limit the compilation targets available to the code. Because this allows the compiler to warn developers faster.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like it will still try to import symbols if you actively reference it.

I think you may need to run cargo update. Since this is only released in 0.2.88 last month.

I tried the following code:

fn main() {
    js_sys::Date::now();
}
image

Personally, I feel that it is not a bad thing to proactively limit the compilation targets available to the code. Because this allows the compiler to warn developers faster.

This is a design decision made all the way up to wasm-bindgen.
One of the reasons is that you can write code targeting both wasm (client) and native targets (server) at the same time without having to reconfigure toolchain.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

@langyo Since 0.2.88 solves the WASI issues, would it be possible to create a pull request to remove the feature flags from gloo-history so we do not subsequently introduce more feature flags into Yew?

Copy link
Contributor Author

@langyo langyo Dec 9, 2023

Choose a reason for hiding this comment

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

@langyo Since 0.2.88 solves the WASI issues, would it be possible to create a pull request to remove the feature flags from gloo-history so we do not subsequently introduce more feature flags into Yew?

I think it's better not to remove it yet. Keeping some flags will help users who compile with WASI to quickly locate the cause of the error.

But I will actively consider this matter... Perhaps there's no need to resort to strong measures to circumvent this problem. 🤔

Copy link
Collaborator

@futursolo futursolo Dec 9, 2023

Choose a reason for hiding this comment

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

Keeping some flags will help users who compile with WASI to quickly locate the cause of the error.

I think it is important to ensure consistency between gloo and wasm-bindgen handling.

When users wrongly uses gloo-history under wasi with client side rendering code, they pretty much uses wasm-bindgen and web-sys wrongly as well. So the user will inevitably be facing runtime errors even if gloo-history uses compiler flags.

I have tried to apply a similar approach with target flags to make Yew Send, and it resulted in a very large number of feature flags being used and it will require every user who writes SSR to apply a large number of feature flags.
I have reached a conclusion that using feature flags as unfeasible for end users.

The current approach of wasm-bindgen, gloo and Yew allow users to avoid compiler flags in most situations whlist still be compatible with both browser and native targets.

In addition, it will eventually be possible for wasm32-wasi to run under browser environments.
These feature flags will need to be removed to provide that support, which will be another breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your opinion is also very reasonable. I thought about it for a while and I'll try to roll back a part of the code later.

On the possibility of WASI running in the browser. Personally, I don't think it would be appropriate for WASI to be a well-designed virtual machine interface that would be arbitrarily introduced as a non-standard interface. It's not for technical reasons, but because of some historical lessons... If some non-standard interfaces are adopted on a large scale, it will be very troublesome to standardize for them in the future.

As a recent example, wasmer has actually made an interface that is slightly different from standard WASI. Even though this made it become the first WASM runtime written in Rust to be able to communicate over the network, it came at the cost of introducing a large number of specialized patch libraries, even including the Rust compiler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the possibility of WASI running in the browser.

Browsers are already on track to support WASI.
What is not yet decided is that whether and how wasm-bindgen wants to support wasi.

https://bugzilla.mozilla.org/show_bug.cgi?id=1701197

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

3 participants