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

Remove .unwrap() from JS API; Driver.new => new Driver #142

Merged
merged 18 commits into from Dec 14, 2021
Merged

Conversation

cormacrelf
Copy link
Collaborator

@cormacrelf cormacrelf commented Dec 5, 2021

This is based on my nearly-finished wasm-bindgen PR rustwasm/wasm-bindgen#2710. It works!

It completely removes the WasmResult workaround for wasm-bindgen# 1963 that I devised previously, and all the annoying and difficult to remember .unwrap() calls. A number of other workarounds are also no longer necessary:

  • Driver.new does not need to be a method, it can be a constructor i.e. new Driver
  • A bunch of code in the wasm crate for giving Typescript definitions to all those return types, which would have otherwise been just WasmResult instead of WasmResult<BibliographyMeta> etc.
  • Docs on this pattern removed
  • Removed all the uses of it from the tests etc

So the upgrade guide for this is

  1. replace Driver.new with new Driver
  2. delete occurrences of .unwrap()

cargo install fastmod is always helpful for this.

Misc

  • add cancellation to prevent multiple in-flight workflows on same PR/branch
  • add cslFeatures to typescript
  • temporarily use precompiled wasm-bindgen from my fork
  • dramatically simplify the js-demo project by using esbuild instead of webpack

@cormacrelf cormacrelf added A-wasm Area: wasm package on npm breaking Breaking change enhancement New feature or request labels Dec 5, 2021
@cormacrelf cormacrelf changed the title Remove .unwrap() from JS API Remove .unwrap() from JS API; Driver.new => new Driver Dec 5, 2021
@cormacrelf cormacrelf added the A-ci Area: continuous integration label Dec 5, 2021
@cormacrelf
Copy link
Collaborator Author

cormacrelf commented Dec 13, 2021

Update, my wasm-bindgen PR was accepted, so we can commit to this change. It will presumably be released in version 0.2.79, but so as not to hold things up I'm going to merge this almost as-is, including the workaround for building the WASM.

Please note that for the time being, using wasm-pack alone will not successfully build a copy of @citeproc-rs/wasm until we're back on mainline wasm-bindgen, as wasm-pack must go off and fetch a pre-compiled version to match, and when it does, there will be a mismatch between the annotations embedded in rustc's .wasm file, the format expected by the mainline 0.2.78 wasm-bindgen tool, and the code it will generate, with the most likely result that wasm-bindgen simply crashes. The build scripts all rely on my fork being installed and on your $PATH already, and the CI has a routine that goes and downloads one I uploaded to a github release.

If you're just using @citeproc-rs/wasm from NPM, then that's irrelevant to you. .unwrap() is dead, long live throwing errors like a normal JS function.

@cormacrelf cormacrelf marked this pull request as ready for review December 13, 2021 18:26
@adomasven
Copy link
Member

Awesome! This is great news.

ci: add wasm32-unknown-unknown target
ci: duplicate wasm-bindgen downloader
@cormacrelf cormacrelf force-pushed the no-unwrap branch 2 times, most recently from 90c5c69 to 2cb6a65 Compare December 14, 2021 14:34
@cormacrelf
Copy link
Collaborator Author

cormacrelf commented Dec 14, 2021

This now includes a workaround for npm/cli#4126, which has caused a couple of canary builds to fail to publish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ci Area: continuous integration A-wasm Area: wasm package on npm breaking Breaking change enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

2 participants