-
Notifications
You must be signed in to change notification settings - Fork 146
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
Conversation
cc @hamza1311 😉 Maybe it's time to release a patch version in cargo that supports Related to yewstack/yew#3534 |
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 taking the time to do this
Released in gloo-net 0.5 🎉 |
@langyo gloo-net 0.5 has been released. Can you update this PR? |
@@ -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"))] |
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.
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.
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.
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.
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 think WASI has been disabled in rustwasm/wasm-bindgen#3233?
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 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.
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.
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();
}
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.
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.
@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?
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.
@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. 🤔
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.
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.
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.
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.
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.
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.
#405
It's ready to review now.