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

Replace wasm-pack #1836

Open
max-sixty opened this issue Feb 14, 2023 · 2 comments
Open

Replace wasm-pack #1836

max-sixty opened this issue Feb 14, 2023 · 2 comments
Labels
devops postponed

Comments

@max-sixty
Copy link
Member

Not the most urgent item, but would be good to keep our build steps as uncomplicated as possible, and remove dependencies that aren't maintained.

Currently we use wasm-pack to bundle prql-js. IIUC we're using it because

  • we previously wanted to create a simple npm package, which wasm-pack does well
  • then we decided to also create bundler & web targets
  • so we made our own npm package which contained all three
  • ...but Instead of using wasm-bindgen alone, we kept wasm-pack and added package.json scripts to move the files it output (link above)

For context, wasm-pack is a wrapper of wasm-bindgen and isn't really maintained, whereas wasm-bindgen remains well-supported.

As discussed in https://rustwasm.github.io/docs/wasm-bindgen/reference/deployment.html, I think it should be possible to remove this middle layer and use wasm-bindgen alone; likely using a similar set of steps to what we do now in package.json scripts.

This would be a good contribution for someone who's somewhat familiar with JS, and would like an early PR before diving into the PRQL compiler.
@max-sixty max-sixty added help wanted javascript good first issue Tasks suited for learning the compiler codebase labels Feb 14, 2023
@max-sixty max-sixty mentioned this issue Feb 19, 2023
Merged
max-sixty added a commit to max-sixty/prql that referenced this issue Feb 19, 2023
@max-sixty
build(js): Fix wasm-pack's use of .gitignore
1160c60 
Closes PRQL#1875. Not sure why this used to work and doesn't work, but the `.gitignore`s were causing no files to be added to the package. Now we hackily remove them.

This event adds weight to PRQL#1836
max-sixty added a commit that referenced this issue Feb 19, 2023
@max-sixty
build(js): Fix wasm-pack's use of .gitignore (#1887)
d843658 
Closes #1875. Not sure why this used to work and doesn't work, but the `.gitignore`s were causing no files to be added to the package. Now we hackily remove them.

This event adds weight to #1836
@max-sixty
Copy link
Member Author

There's now a library that will do build a wasm package as part of the build:

Quoting from #2158

https://crates.io/crates/substrate-wasm-builder

It requires nightly, which we won't ever require, but also possibly it might be moving to not require nightly? paritytech/substrate#13580

We're 2-3 months behind on the toolchain, so assuming this works, we could use this in a few months to replace wasm-pack. That would simplify the build a lot as well as solving this perf issue.

So I'll mark this as postponed, but leave it open, and we can implement this when it's available. It'll make the builds faster, much much faster when there are no changes, and reduce our use of unsupported crates.

@max-sixty max-sixty added postponed and removed help wanted good first issue Tasks suited for learning the compiler codebase labels Mar 23, 2023
@max-sixty max-sixty changed the title Replace wasm-pack with bare wasm-bindgen Replace wasm-pack Mar 23, 2023
@max-sixty max-sixty mentioned this issue May 12, 2023
Closed
2 tasks
@max-sixty max-sixty mentioned this issue Jun 12, 2023
Merged
max-sixty added a commit to max-sixty/prql that referenced this issue Jun 20, 2023
@max-sixty
build: Replace wasm-pack
9d9a840 
This is the beginning of PRQL#1836, replacing `wasm-pack` with https://crates.io/crates/substrate-wasm-builder. `wasm-pack` has been the source of lots of issues, makes builds slower -- in particular small code changes cause a very long iteration cycle in playground builds -- and is no longer maintanied.

It's a bit harder than I thought:
- We need to create the paths `bundler`, `node`, `web` paths ourselves, replacing [these lines](https://github.com/PRQL/prql/blob/5db61f45ad31ff5673fdfb7890cc3699d47940f9/bindings/prql-js/package.json#L20-L21), by calling `wasm-bindgen debug/wbuild/prql-js/prql_js.wasm --target=...`.
- We probably need some way of getting the `.wasm` file location, by reading `include!(concat!(env!("OUT_DIR"), "/wasm_binary.rs"));` for the `WASM_BINARY` or `WASM_BINARY_BLOATY` constants. Ideally we'd do this at build time, all in the `build.rs` script; I'm not sure whether that's possible though.
- Possibly I'm figuring too much of this out on my own and it's done elsewhere? The precedent for this approach seems to be blockchain rather than JS applications...
@max-sixty max-sixty mentioned this issue Jun 20, 2023
Closed
@max-sixty
Copy link
Member Author

max-sixty commented Jun 22, 2023 •
edited
Loading

Update:

  • substrate-wasm-builder is cool but not that practical for us — it doesn't generate the JS file. It compiles a .wasm artifact by creating a whole new crate at build-time, building it, and then copying the .wasm artifact back 1
  • https://github.com/rustminded/xtask-wasm looks good as a replacement but doesn't seem to handle generating the package.json etc, which wasm-pack does
  • More understanding in Building with `build.rs` rustwasm/wasm-bindgen#3494 (reply in thread)
  • So — stepping back — except from the general overhead of having wasm-pack around, the main pain comes from running wasm-opt on each run — it has no caching, and so is run on every run.

So I think for the moment, we can have a build option that runs with --dev, which skips wasm-opt, which we can run locally. And then we can return to this if the ecosystem's tooling improves.

Footnotes

  1. Given that it doesn't create JS files, I'm not sure why it does this, rather than just compiling the main project with wasm — possibly there are parts of the project that want to use a default target but which want to use a .wasm binary. ↩

max-sixty added a commit to max-sixty/prql that referenced this issue Jun 22, 2023
@max-sixty
build: Speed up playground build
4b59cda 
As discussed in PRQL#1836 (comment)
@max-sixty max-sixty mentioned this issue Jun 22, 2023
Merged
max-sixty added a commit to max-sixty/prql that referenced this issue Jun 22, 2023
@max-sixty
build: Speed up playground build
92fdb83 
As discussed in PRQL#1836 (comment)
@srenatus srenatus mentioned this issue Feb 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
devops postponed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@max-sixty @aljazerzen