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.

Sign up for GitHub

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

Jump to bottom

Simple wasm example #801

Merged
merged 3 commits into from
May 31, 2020
Merged

Simple wasm example #801

merged 3 commits into from
May 31, 2020

Conversation

futurepaul
Copy link
Collaborator

@futurepaul futurepaul commented Apr 4, 2020 •
edited
Loading

This depends on #759 and should be merged after (and will def need some changes before that!).

Just wanted to get something up so I could get some feedback. A couple questions:

  1. Should we come up with a new example specifically for the web? Or is hello fine.
  2. Is this doing the Cargo workspace stuff correctly? It's bugging me about needing a license file but I don't see that warning from i.e. druid-shell?

@elrnv
Copy link
Contributor

elrnv commented Apr 4, 2020

FWIW, you should be able to compile and serve almost all the examples unchanged so long as fn main is made pub and std::time::Instant is swapped with instant::Instant in each example.

@futurepaul futurepaul marked this pull request as ready for review April 16, 2020 16:20
@futurepaul futurepaul added the S-needs-review waits for review label Apr 16, 2020
cmyr
cmyr approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I haven't tried building it. @elrnv any input?

luleyleo
luleyleo requested changes Apr 17, 2020
View reviewed changes
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

Looks good, the readme instructions work.
Seems to have unused dependencies tho.

druid/examples/hello_wasm/Cargo.toml Outdated Show resolved Hide resolved
@cmyr cmyr removed the S-needs-review waits for review label Apr 17, 2020
@elrnv
Copy link
Contributor

elrnv commented Apr 17, 2020

This builds and runs for me perfectly :)

I would add two things.

  1. Should we have a CI build for this example for completeness?
  2. Druid could be used for other WASM targets like Electron. This particular use case builds it for the web target to be used in browsers, but the Electron build is slightly different. With that in mind I would suggest renaming this to hello_web or hello_wasm_web for the possibility of adding hello_npm or something similar later for other wasm targets. I think having examples for other wasm targets is important because the behaviour of windows and menus could be different.

xStrom
xStrom requested changes Apr 18, 2020
View reviewed changes
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

As this is a new package it needs to also be added to the CI.

It's pretty straightforward, look into the ci.yml file and copy the druid-wasm-examples clippy/build steps.

@futurepaul futurepaul added the S-waiting-on-author waits for changes from the submitter label Apr 19, 2020
@futurepaul futurepaul added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels May 27, 2020
@futurepaul
Copy link
Collaborator Author

@Finnerale @xStrom sorry for the long delay but I've finally addressed your comments. Not sure why the Cargo.lock is conflicting, it wasn't yesterday.

@xStrom
Copy link
Member

xStrom commented May 27, 2020

It's conflicting because it doesn't exist anymore. You need to rebase with master and make sure Cargo.lock doesn't exist in your PR.

luleyleo
luleyleo reviewed May 27, 2020
View reviewed changes
Copy link
Collaborator

@luleyleo luleyleo 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 continuing this!
Some more nitpicking, but then it should be good to go.

druid/examples/hello_wasm_web/README.md Outdated Show resolved Hide resolved
druid/examples/hello_wasm_web/src/lib.rs Outdated

const VERTICAL_WIDGET_SPACING: f64 = 20.0;
const TEXT_BOX_WIDTH: f64 = 200.0;
const WINDOW_TITLE: LocalizedString<HelloState> = LocalizedString::new("Hello World!");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Window titles can be normal strings now 🎉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks like window title isn't used by the web version so I'm leaving it off entirely now

druid/examples/hello_wasm_web/src/lib.rs Outdated Show resolved Hide resolved
druid/examples/hello_wasm_web/src/lib.rs
// hello.rs example.
#[wasm_bindgen]
pub fn wasm_main() {
std::panic::set_hook(Box::new(console_error_panic_hook::hook));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice if this one could get a comment, explaining that this is necessary to get panic messages on wasm32.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was unaware of what this actually did, so I hope my copy of your explanation is sufficient.

@xStrom xStrom added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels May 29, 2020
@futurepaul
Copy link
Collaborator Author

Okay, I believe I've addressed all comments. Also, I realized I hadn't updated the index.js to refer to the different package name, so that's fixed now as well.

xStrom
xStrom approved these changes May 31, 2020
View reviewed changes
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Looks like a good start.

@xStrom xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels May 31, 2020
luleyleo
luleyleo approved these changes May 31, 2020
View reviewed changes
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

Looks good!

@luleyleo luleyleo removed the S-needs-review waits for review label May 31, 2020
@futurepaul futurepaul merged commit a4b0762 into linebender:master May 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luleyleo luleyleo luleyleo approved these changes

@cmyr cmyr cmyr approved these changes

@xStrom xStrom xStrom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@futurepaul @elrnv @xStrom @cmyr @luleyleo