Breaking JS changes: Driver.new(InitOptions); WasmResult wrapper/.unwrap(); parseStyleMetadata #94
Conversation
…s stack overflows
This way we never call wasm_bindgen's throw_val, which doesn't clean up after itself.
Before:
driver.batchedUpdates()
After:
driver.batchedUpdates().unwrap() // may throw an error
The object with the unwrap() method is shaped like { Ok: T } or { Err: Error }.
This object is included in `crates/wasm/src/include.js`, which most builds can
import/bundle by themselves, but not no-modules, which doesn't work yet.
... for constructor (InitOptions) & setStyle
Not sure if this is right, but ok
I remembered it was necessary in the browser/esm/node code as well. At least now the TypeScript defines them as globals, so that mistake is more obvious.
Wasn't actually necessary to solve #78 But nevertheless.
So this build script is getting a bit involved, but that's the fragmented JS module ecosystem for you.
This is unnecessarily clunky on the client side of the Driver. Can't it just return the value or throw whatever error it was going to throw, i.e. call |
|
Right, it can throw errors automatically by using wasm-bindgen’s generated code for this but not without leaking stack space in the code that I don’t control. I didn’t do it just to give you this not very javascripty API. There are no benefits apart from not having a memory leak. As for “just throw them (manually)”, I can’t because throwing an error uses the same leaky method as the glue code. Simply: if you throw a JS error inside the WebAssembly code, it never bubbles back up through the WebAssembly, because it is not a JS VM stack! The JS VM cannot unwind it. So it never resets the stack pointer. AFAIK this kind of thing may be the only way to solve rustwasm/wasm-bindgen#1963 at all. Once we have ESR 78 running and a couple of WebAssembly spec items are no longer missing, and so we’re no longer pinned to an old wasm-bindgen, then maybe all this goes away if I can upstream the solution in some way that makes it throw them automatically again but from outside the WASM stack. That seems pretty viable to me, I’ll report on this upstream. I can write a complete wrapper ie “unwrapper” class that does this for you if you really want but it is only about ten unwraps, vs a long wrapper class that would have to be kept in sync. |
|
It isn't strictly necessary given that this is primarily a Rust implementation of citeproc, but if there's WASM JS bindings they may as well take the extra step and do this sort of thing. If we're going out of the way to make the API more convenient for current and future clients in other issues, making it clunky like this here is a step back. This isn't top priority, since I imagine it is not that easy to do given all the different environments that you're generating for means that adding a wrapper would be quite a bit of work, but something certainly worth considering. |
|
In the interests of not breaking again for reasons people don't care about, yeah. |
cormacrelf
added
A-wasm
The diff on the wasm README should give you a good idea of what's changed here. I've bundled up all the breakages in one go, hopefully that is more convenient than one new breakage every day.
InitOptions, to accomodate optional configurations and flags.parseStyleMetadata. Useful for parsing dependent styles especially, but works for all.await driver.fetchAll()toawait driver.fetchLocales(), documented betterWasmResult, to avoid potential stack problems with wasm-bindgen's leaky implementation of automatic error-throwing. You can get errors thrown again with.unwrap()after each call, which is a good default because otherwise these errors will just sit there unnoticed. It's not the greatest, but it avoids a memory leak and stack overflow.CiteprocRsError:CslStyleErrorandCiteprocRsDriverError. Both are globals (or in the case of Zotero, added toZotero.CiteprocRs.*)citeproc_rs_wasm_include.js.replace("export class", "class")hacks. It places exports onZotero.CiteprocRsas well asmodule.exports.