-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Return immediately from run_app
on web
#4165
base: master
Are you sure you want to change the base?
Conversation
I think they worked properly as well? Just we had a hack to hold the exit? In general, the point was that you might have a code that does cleanup after the exit in a cross platform manner, and thus, doing an instant return doesn't make much sense from a cross platform behavior. The iOS model is acceptable, since you close the app anyway after so. In general, I'm leaning towards special backends just not being a part of the regular run facility at all, so the difference is clearly stated for them. |
The hack was "throw an exception and cross fingers that Rust doesn't see it and run destructors". Not really what I'd call "worked properly", rather "terribly UB but worked because WASM doesn't yet support exceptions".
On the web and iOS currently, having cleanup after
My problem is that example code suffers when we do not provide a single API that everyone can reliably use (Android is special in that it cannot be a binary, but the others do not need to suffer from this). An alternative would be to provide |
This avoids using JavaScript exceptions to support `EventLoop::run_app` on the web, which is a huge hack, and doesn't work with the Exception Handling Proposal for WebAssembly: https://github.com/WebAssembly/exception-handling This needs the application handler passed to `run_app` to be `'static`, but that works better on iOS too anyhow (since you can't accidentally forget to pass in state that then wouldn't be dropped when terminating).
ca7e746
to
7cbc584
Compare
Yeah, but those targets are rather special, so I don't see an issue with them being treated via their own |
An alternative would be to provide a As a few data points, Bevy is already effectively merging My point is that all of these examples would benefit from this PR. |
I do somewhat agree here, the differing semantics are confusing (I've tried to remediate this with better documentation), though I guess my point is that to most users it probably won't matter (either they only support desktop, and then they don't care, or they also support web, in which case they'll have tested their app there and seen that it does indeed work as they expect). |
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 don't like this solution at all, but I think its the best we can do right now.
Thank you for doing this!
@@ -194,6 +194,8 @@ changelog entry. | |||
- Removed `KeyEventExtModifierSupplement`, and made the fields `text_with_all_modifiers` and | |||
`key_without_modifiers` public on `KeyEvent` instead. | |||
- Move `window::Fullscreen` to `monitor::Fullscreen`. | |||
- On web, avoid throwing an exception in `EventLoop::run_app`, instead preferring to return to the caller. |
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 web, avoid throwing an exception in `EventLoop::run_app`, instead preferring to return to the caller. | |
- On Web, avoid throwing an exception in `EventLoop::run_app`, instead preferring to return to the caller. |
if self.0.event_loop_recreation.get() { | ||
crate::event_loop::EventLoopBuilder::allow_event_loop_recreation(); | ||
} | ||
crate::event_loop::EventLoopBuilder::allow_event_loop_recreation(); |
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.
We should mention in the changelog that event loop recreation is now always allowed on Web.
Wondering, in the ideal world, how would you like Winit's cross-platform entry point to be? |
So in the future, with the Because of iOS not able to return after the If we can't have a cross-platform API that behaves the same on every platform, my next preference is to just not have a cross-platform API. One argument against that is that users often just ignore the differences like this: #[cfg(not(target_family = "wasm"))]
event_loop.run_app(App::default())?;
#[cfg(target_family = "wasm")]
event_loop.spawn_app(App::default()); So we agreed that if we can't find a truly cross-platform API, we will go ahead with this PR's proposal. |
To expand on iOS: The only public API that Apple provide is I can think of several workarounds to return to the user's code anyhow upon termination, but all of them have costs that I don't think Winit should pay.
|
Builds upon #4149.
Returning immediately from
EventLoop::run_app
on the web avoids using JavaScript exceptions, which is a huge hack that has compatibility issues, and doesn't work with the Exception Handling Proposal for WebAssembly.This needs the application handler passed to
run_app
to be'static
, but that works better on iOS too anyhow (since you can't accidentally forget to pass in state that then wouldn't be dropped when terminating). This effectively reverts the decision in #3006, CC @kchibisov, do you recall if there was a deep motivation for doing that?Since
spawn_app
(added in #2208) is now no longer necessary, I've removed it. This means that all the examples should work properly on web again.changelog
module if knowledge of this change could be valuable to users