Use wasm-bindgen instead of stdweb #6
Conversation
|
Very interesting! Yes, after reading the linked issue I agree that wasm-bindgen is a better fit for the goals of piet, though the current convenience of stdweb also has its value. I noticed that my hello example was 130k, which is not horrific but also not great. It seems likely that wasm-bindgen will push this firmly into the "great" category, which is exciting. Basically the main thing I ask is that the instructions for building the demo are super clear. But you probably understand the importance of that. Thanks for working on this! |
|
One more thing might be worth mentioning: it's not yet clear to me whether web 2d canvas or svg is ultimately going to be more useful. Of course, an svg backend would make sense both in web and non-web contexts, in the latter case mostly spitting out a string (though some kind of dom-like tree would also be an option). But I don't want to start too many backends until the core traits have stabilized :) |
|
@raphlinus Awesome! I pushed up an example. It not quite as easy as before to get running but if you have npm already installed it's basically just as many steps. A possible advantage for some people is that it mirrors a bit more closely a flow you'd be used to if you're coming from the web world. I don't love that the cargo workspaces exception I had to put in the Cargo.toml file, but unfortunately I don't have a ton of experience with workspaces so I couldn't figure out a better way to do it in the limited time I had. The wasm file is 90kb smaller on my machine when compared to the previous example. I'm sure we could do better with some additional steps. Thoughts? |
|
This should be good to go. There's more we can do to make the lib a bit more flexible for users. With regards to code size, there's much more we can do depending on how willing we are to sacrifice some performance especially if there is little to no need for dynamic allocation. You can read more about that here: https://rustwasm.github.io/book/reference/code-size.html |
piet-web/README.md
Outdated
|
|
||
| `$ cargo install -f cargo-web` | ||
| `$ cargo install -f wasm-bindgen` |
There was a problem hiding this comment.
I get this:
$ cargo install -f wasm-bindgen
Updating crates.io index
Installing wasm-bindgen v0.2.30
error: specified package has no binaries
I can diagnose this, but I have on my "hapless dev" hat. This is on a Windows 10 box in a git bash shell, and node (10.15.0) and npm are installed.
There was a problem hiding this comment.
Whoops! It's actually wasm-bindgen-cli. Once it's installed it's just called wasm-bindgen. I will update this.
piet-web/examples/basic/build.sh
Outdated
|
|
||
| # Finally, package everything up using Webpack and start a server so we can | ||
| # browse the result | ||
| npm install --prefix basic-web-static |
There was a problem hiding this comment.
This also has a paper-cut error: works fine if I cd to basic-web-static and do npm install and serve from there, but not from the dir as shown in the batch file. But then it works!
There was a problem hiding this comment.
Hmm... the --prefix option should take care of that. I don't need to cd into that directory on my machine. Maybe different versions of npm are causing this?
I can change the script to cd into that directory instead. Do you think that will be a more robust solution? (ah... the joys of bash scripting ❤️)
There was a problem hiding this comment.
The world of npm is foreign and strange to me, though I have played with it some. Looking at the docs, it looks like prefix sets the path where the artifacts get stored, but not where the package.json is read from. Did you try this from a fresh clone? (my inexperienced experience is that npm tends to litter a lot of files in your working directory)
I'm fine with it cd'ing into the directory. In general I would want to follow "best practices" but have no idea what those are. Not to be too nitpicky, but I think this is kinda important because a lot of people start projects by copying a hello example.
There was a problem hiding this comment.
No worries on being nitpicky. I 100% that this should be painless for people.
What you said about --prefix is true for global items (installed with -g) but for local cli commands this tells npm in which directory to run. From the docs: "If set on the command line, then it forces non-global commands to run in the specified folder."
I tried from a fresh clone and it worked fine for me.
If you run npm install --prefix basic-web-static from that directory (the $PROJECT_ROOT/piet-web/examples/basic directory), it warns that you don't have a package.json?
I try to avoid using cd in my bash scripts because I've ran into weird issues before, but if that's what's needed to get it working, we can do it.
There was a problem hiding this comment.
raphl@DESKTOP-6JFLCBE MINGW64 ~/github/piet/piet-web/examples/basic (rylev-wasm-bindgen)
$ npm -v
6.4.1
$ npm install --prefix basic-web-static
npm ERR! code ENOLOCAL
npm ERR! Could not install from "" as it does not contain a package.json file.
npm ERR! A complete log of this run can be found in:
npm ERR! C:\Users\raphl\AppData\Roaming\npm-cache\_logs\2019-01-07T17_39_10_724Z-debug.log
I'm not sure what's going on here, and totally understand wanting to minimize cd'ing.
There was a problem hiding this comment.
Looks like a bug in the windows version of npm: https://npm.community/t/npm-prefix-flag-does-not-work-as-expected-on-windows/3147
I think moving to just using cd is the best choice.
There was a problem hiding this comment.
Sounds good - I was just about to try it on my Mac, but seems like you've isolated it. That bug is scary, being auto-closed even though it seems valid and with the potential to affect lots of people.
There was a problem hiding this comment.
That's unfortunately how the web world works. It's pretty fast and loose. The latest commit should fix it (though I just ran into another way things could break that the readme now references).
There was a problem hiding this comment.
Ok, I think everything is good now. We can fine-tune as needed later.
What I generally like to do is give commit access and let the author merge, so that there's a chance to get any last minute afterthoughts in.
Thanks again for this. I'm quite excited about the potential for this stuff to not just work on the web, but work well on the web.
|
Awesome. FYI: I have very little experience in graphics, but I hope I can learn through this repo. I'll try to be as helpful as possible where I can be. |
|
@raphlinus I got the npm team to change the issue I posted above to a bug report. If you want you can add your OS information as well. https://npm.community/t/npm-prefix-flag-does-not-work-as-expected-on-windows/3147 |
This is still a work in progress, but I wanted to see what you thought...
stdweb is fairly heavy handed. wasm-bindgen and the related web-sys crate provide the bare minimum for integrating Rust in a JavaScript and browser environment. These are the crates that the Rust Wasm working group is focused on. There is an issue calrifying the differences. The stdweb maintainer hints at interest in eventually having stdweb be built on top of wasm-bindgen.
The changes are fairly minimal, but it should set the project on a path of being as minimal as possible. The downside to this approach is that it makes the example a bit more involved to run since
cargo webdoes not currently work with wasm-bindgen.If you're interested in this approach I will get the example running.