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 explaination of the raw wasm module #42

Merged
merged 6 commits into from
Jul 16, 2018
Merged

Add explaination of the raw wasm module #42

merged 6 commits into from
Jul 16, 2018

Conversation

alfiedotwtf
Copy link
Contributor

In reference to #38

@alfiedotwtf alfiedotwtf changed the title Add explaination to the raw wasm module Add explaination of the raw wasm module Jul 6, 2018
Copy link
Member

@fitzgen fitzgen 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 cleaning a lot of this stuff up @alfiedotwtf :)

I have a few nitpicks inline below that we should fix before merging. Also some things that I think @ashleygwilliams or @mgattozzi should sign off on regarding the wasm-pack tutorial bits.

Thanks!

cell is dead or alive, respectively. By working with pointers and overlays, we
avoid copying the cells across the boundary on every tick.
We can directly access WebAssembly's linear memory via `memory`, which is
defined in the raw wasw module `wasm_game_of_life_bg`. To draw the cells, we
Copy link
Member

Choose a reason for hiding this comment

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

typo: "wasw"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh!

have to worry about it too much and just write Rust code for the most part. If
you want to know the full extent of its capabilities check out the README on
its repo which can be found
[here](https://github.com/alexcrichton/wasm-bindgen). For our purposes we need
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: the canonical repo is https://github.com/rustwasm/wasm-bindgen now

you want to know the full extent of its capabilities check out the README on
its repo which can be found
[here](https://github.com/alexcrichton/wasm-bindgen). For our purposes we need
to know that if we want functions to work with wasm easily we'll need it.
Copy link
Member

Choose a reason for hiding this comment

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

This last sentence isn't very clear to me.

In fact, I think we can replace these first two paragraphs with simply:

> We will use [wasm-bindgen](https://github.com/rustwasm/wasm-bindgen) to
> communicate between Rust/wasm and JavaScript. `wasm-bindgen` currently
> relies on nightly-only features (`proc_macro`, etc...) that we must opt
> into using.

I want to keep the intro very short and to the point because people bounce off of walls of text, especially when first getting started before they are hooked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

[here](https://github.com/alexcrichton/wasm-bindgen). For our purposes we need
to know that if we want functions to work with wasm easily we'll need it.

The `extern crate` call lets the compiler know what crates to link in and the
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: "call" is not the right word here, because it isn't a function or method.

We could either completely drop it, or replace with "declaration".

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 agree to removing this.

@@ -28,8 +28,8 @@ Calling `performance.now` has very little overhead, so we can create simple
measurements from it without distorting the performance of the rest of the
system.

For example, we can create a simple FPS counter that we update on each iteration
of our `renderLoop`:
For example, we can create a simple FPS (frames per second) counter that we
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 flip this to "frames per second (FPS)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

prelude from `wasm-bindgen`. The `extern crate` call lets the compiler know what crates to link in
and the `prelude` contains all the types and functions that `wasm-bindgen` needs to work properly!

Cool let's import the `alert` function from JS so that we can call it in our Rust code!
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure whether we want to remove all this and what level of wasm-bindgen familiarity the wasm-pack tutorial is expecting.

+cc @ashleygwilliams @mgattozzi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. I've put them back in.

@alfiedotwtf
Copy link
Contributor Author

I think that's everything. Let me know if you find anything else needing changes 👍

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @alfiedotwtf!

Would still like to hold off on merging before @ashleygwilliams signs off on the wasm-pack tutorial changes.

@alfiedotwtf
Copy link
Contributor Author

I've dropped the wasm-pack tutorial commits, so that the rest can be merged. I'll submit a separate PR for them so that these don't go stale.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks @alfiedotwtf :)

@fitzgen fitzgen merged commit 1039c48 into rustwasm:master Jul 16, 2018
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.

2 participants