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 WASM CustomEvent Example #2852

Closed
wants to merge 14 commits into from
Closed

Add WASM CustomEvent Example #2852

wants to merge 14 commits into from

Conversation

stevenhuyn
Copy link

@stevenhuyn stevenhuyn commented Jun 5, 2023

The current CustomEvent example in the ./examples folder doesn't show how to do it via WASM.

Atm I'm just printing to console, however we could reuse the examples/web.rs code to append to a HTML list. But I wanted to keep the example a bit more streamline.

Also I added some extra CSS to centre align it. The button will be placed on the right of the canvas otherwise. lmk if it's uggo and I'll revert it.

image

Closes #2816
Answers #2061 (comment)

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

@rukai I guess I didn't really think my last comment through (#2816 (comment)) , but you can't really show the JS in the generated HTML file because it's inserted during runtime, or is there something cargo-run-wasm can do about that I'm not aware of?

Additionally, it would also be really nice to not have to build a bunch of HTML through JS or web-sys. Again, none of this can actually be shown to the user because it's all added during runtime.

Comment on lines 29 to 32
// Because EventLoopProxy is not Send, we need to wrap it in a RefCell and use thread_local!
thread_local! {
pub static EVENT_LOOP_PROXY: RefCell<Option<EventLoopProxy<CustomEvent>>> = RefCell::new(None);
}
Copy link
Member

Choose a reason for hiding this comment

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

Since #2834 it is actually!
In any case it's still appropriate to use thread_local! if we aren't doing anything with web workers.
We should also probably use once_cell::unsync::OnceCell instead of RefCell.

Copy link
Author

Choose a reason for hiding this comment

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

I am not certain want to make the comment now. Not super familiar with web workers. (js is single threaded unless you use web-workers is my understanding (?))

Comment on lines +34 to +42
// Function to be called from JS
fn wasm_call() -> u32 {
42
}

#[derive(Debug, Clone, Copy)]
pub enum CustomEvent {
WasmCall,
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's make a slightly more useful example. Instead of just printing 42 we could e.g. use Window::set_inner_size(), which is a valid use-case and would also show how to pass parameters. This would require two input fields for width and height and a button to send.

Copy link
Author

Choose a reason for hiding this comment

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

Awaiting if the cargo-wasm-runner will go through first. rukai/cargo-run-wasm#36

Comment on lines +66 to +69
Event::WindowEvent {
event: WindowEvent::CloseRequested,
window_id,
} if window_id == window.id() => control_flow.set_exit(),
Copy link
Member

Choose a reason for hiding this comment

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

We can skip that here, it's not a thing in Wasm.

Copy link
Author

Choose a reason for hiding this comment

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

Skip this match arm?

Event::WindowEvent {
    event: WindowEvent::CloseRequested,
    window_id,
} if window_id == window.id() => control_flow.set_exit(),

@daxpedda daxpedda marked this pull request as draft June 5, 2023 09:41
@rukai
Copy link
Contributor

rukai commented Jun 5, 2023

@rukai I guess I didn't really think my last comment through (#2816 (comment)) , but you can't really show the JS in the generated HTML file because it's inserted during runtime, or is there something cargo-run-wasm can do about that I'm not aware of?

Oh uh... true. 😅
Scratch that idea then.

Next best thing I can think of would be just manually creating an unused html file with the appropriate javascript in it :/

@daxpedda
Copy link
Member

daxpedda commented Jun 5, 2023

Next best thing I can think of would be just manually creating an unused html file with the appropriate javascript in it :/

Would you accept a feature request extending the API of cargo-run-wasm with something like with_html_body()?

@rukai
Copy link
Contributor

rukai commented Jun 5, 2023

Yep, I think we do need this feature, I've raised rukai/cargo-run-wasm#35
Not sure when I'll get to implementing it.

@stevenhuyn
Copy link
Author

stevenhuyn commented Jun 5, 2023

I could consider looking into adding that feature. Looks relatively straightforward via web_sys

@daxpedda daxpedda self-assigned this Jun 5, 2023
@daxpedda daxpedda added the C - waiting on author Waiting for a response or another PR label Jun 5, 2023
@daxpedda
Copy link
Member

daxpedda commented Jun 6, 2023

@stevenhuyn you should keep the conversation you are having in #2816 (comment) here!
But yeah, I guess that's the best we can do right now. I will take a closer look at it during review.

@stevenhuyn
Copy link
Author

stevenhuyn commented Jun 6, 2023

Ah thought the issue is the ground truth, although I guess implementation details should be in this PR (?)

I have hooked it up to test (pointing cargo run wasm crate to my fork) and have it all working.

Need to look into why CI failing, cause it builds perfectly fine on my machine.

@stevenhuyn stevenhuyn closed this by deleting the head repository Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on author Waiting for a response or another PR DS - web
Development

Successfully merging this pull request may close these issues.

Add WASM CustomEvent Example
3 participants