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

Typescript definition for wasm target #113

Closed
H-Plus-Time opened this issue Feb 2, 2024 · 4 comments · Fixed by #114
Closed

Typescript definition for wasm target #113

H-Plus-Time opened this issue Feb 2, 2024 · 4 comments · Fixed by #114
Labels
enhancement New feature or request wasm-edition The JS library, including the unplugin library

Comments

@H-Plus-Time
Copy link
Contributor

This one comes in two parts:

The trivial one: At the moment, it looks like unbuild is losing the wasm-bindgen'd .d.ts files - flipping the declarations flag on in the builds section was enough (though it does produce a .d.ts, .d.mts per entrypoint, alongside the shared/ezno_lib.d.ts 🤷 ).

Less trivial: better type hinting on the serialized return results in src/wasm_bindings.rs.

I did this a few days ago over in the oxc project (oxc-project/oxc/#2158), it more or less boils down to:

  1. Derive Tsify on bindgen'd structs in src/wasm_bindings.rs (or custom typescript sections for function defs)
  2. Look for pub enums and structs with derive(Serialize) present
  3. Stick either of #[cfg_attr(target_family = "wasm", derive(tsify::Tsify))] or #[cfg_attr(feature = "wasm", derive(tsify::Tsify))] on them (depending on stylistic preference - chrono uses target_family, zstd uses a 'wasm' feature flag).
  4. Repeat 2-3
  5. For structs that absolutely will not auto-derive, are opaque (like anything behind bitflags!), add a #[wasm_bingen(custom_typescript_section)] static text block manually specifying their type (a handful or so cropped up in the oxc PR).

I'll put up a PR shortly, but in the meantime I have a couple of questions:

  1. I'm thinking of adding an unresolved type test on the ezno_lib.d.ts file, partly to catch regressions - something like https://github.com/plantain-00/type-coverage should do the trick, but it could be cool to run it through ezno's own wasm build.
  2. Would you be averse to adding the macro_rules_attribute crate, and the addition of a few shorthands (more or less bundling the serialize, tsify, and probably SelfRustTokenize)?
@kaleidawave
Copy link
Owner

kaleidawave commented Feb 4, 2024

This looks great! Some hopefully helpful information

  • Yes, at the moment the .d.ts built files are not exported. I can't quite remember, but I think when I tried it last I got an error or something and skipped it, just to get it published. If you don't get an error that is great!
  • So at the moment there are two exported areas that would need definitions under:
    • The type checking, this takes TypeCheckOptions and outputs a Vec<Diagnostic> (this type might be lost through JSValue annotations). Should be okay to add Tsify (I am correct in thinking this is a proc macro that creates TS definitions from Rust items?) to those definitions that implement serde::Serialize in the ./checker crate. Additionally the check and build functions (in wasm_bindings) take a file retrieval function. It is typed in Rust as a js_sys::Function, the TS type is (path: string) => string
    • Some parser playground things. I think there are two exports parse_module and parse_expression. Both take ParseOption and they return AST structures (and their many decedents).
  • Serializing (/turning to JSON) in the checker and parser is done under the serde-serialize feature flag. I think derive(Tsify) can added under the same conditional attributes
  • Some items have various serde attributes serde attributes that change the tagging, flattening. Does Tsify account for these?
  • These exports return most things inside the crate. However I think the parser contains Spans (positions of where nodes were parsed from the source) and in the the checker, diagnostics have a Span which point to the origin of the issue. These come from (my) source-map crate. If you also want to PR that repo to add it to there I will gladly accept!
  • Didn't know about the macro_rules_attribute crate! This looks amazing and something I was wondering whether it exists (those attributes are getting quite long now in the parser). I didn't think it was possible. Would be excellent if you added that at the same time.

Looking forward to the PR!

@kaleidawave kaleidawave added enhancement New feature or request wasm-edition The JS library, including the unplugin library labels Feb 4, 2024
@H-Plus-Time
Copy link
Contributor Author

H-Plus-Time commented Feb 5, 2024

  • Some items have various serde attributes serde attributes that change the tagging, flattening. Does Tsify account for these?

Yep, though there's one or two idiosyncrasies (namely rename directly on a struct - it tends to mess with references to the original name).

Indeed, it's a proc macro. Yeah, all the top level function exports (members of wasm_bindings) are ineligible for the macro form sadly (the tsify crate is in a... complicated state re maintenance - pretty sure I can modify it to work on functions, but that would require depending on a fork :-/), so it's back to typescript_custom_section for those, looks like this:

#[wasm_bindgen(typescript_custom_section)]
const TYPES: &str = r###"
export function parse_expression(input: string): {Ok: Expression} | {Err: [string, Span]}
export function parse_module(input: string): {Ok: Module} | {Err: [string, Span]}
"###;

The consensus around non-automated use of those is: Good for slowly changing api surfaces; avoid for everything else (because they're susceptible to typedef drift).

Also, one follow-up question:
The direct wasm-bindgen cli calls, those were motivated by problems getting wasm-pack to work correctly, right? If you like, I have a fix for that that I'll cordon off in it's own commit (so we can omit it if necessary).

@kaleidawave
Copy link
Owner

The consensus around non-automated use of those is: Good for slowly changing api surfaces; avoid for everything else (because they're susceptible to typedef drift).

I see, I will bear that in mind for changes. Can you have separate sections? So that rather than one block, there is one above each function to keep it together?

The direct wasm-bindgen cli calls, those were motivated by problems getting wasm-pack to work correctly, right? If you like, I have a fix for that that I'll cordon off in it's own commit (so we can omit it if necessary).

I can't quite remember but I think I had some issues with wasm-pack. One thing is that using the wasm-bindgen CLI directly I can pin the CLI version so it is the same version as the crate (I have had some problems where the GH actions downloaded a CLI with a new patch which wasn't equal to the one crates dependency and so broke). Is there any benefits to doing it in one with wasm-pack?

@H-Plus-Time
Copy link
Contributor Author

I see, I will bear that in mind for changes. Can you have separate sections? So that rather than one block, there is one above each function to keep it together?

Good point, yes, that's permitted (I've actually already switched to that in the parser crate, far too difficult to write otherwise).

I can't quite remember but I think I had some issues with wasm-pack. One thing is that using the wasm-bindgen CLI directly I can pin the CLI version so it is the same version as the crate (I have had some problems where the GH actions downloaded a CLI with a new patch which wasn't equal to the one crates dependency and so broke). Is there any benefits to doing it in one with wasm-pack?

Yeah, the big win from wasm-pack is wasm-opt (~30% uncompressed size reduction (negligible compressed). The optimization runs are, like most compilers, pretty variable wrt execution time reductions - I typically see ~10-15% speed boosts). the wasm-pack maintainers have been pretty good about ensuring the bindgen version precisely matches the crate's version (there's a fair amount of careful Cargo.toml parsing involved).

@kaleidawave kaleidawave linked a pull request Feb 6, 2024 that will close this issue
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wasm-edition The JS library, including the unplugin library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants