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

Allow typescript_type on struct fields. #3953

Open
prideout opened this issue May 11, 2024 · 3 comments
Open

Allow typescript_type on struct fields. #3953

prideout opened this issue May 11, 2024 · 3 comments

Comments

@prideout
Copy link

Motivation

If you annotate struct fields with #[wasm_bindgen(typescript_type = "EntityId")], wasm-bindgen just ignores it, because typescript_type is meant for type declarations in extern "C".

However sometimes you might want to patch the .d.ts declaration emitted by a struct field.

For example, let's say your Rust app has a newtype called EntityId with no associated methods. It just wraps a u64. You probably do not want to expose such a simple type as a JavaScript class. Happily, wasm-bindgen already lets you do this! Here's what you can do:

impl WasmDescribe for EntityId {
  fn describe() {
    <u64 as WasmDescribe>::describe();
  }
}

impl IntoWasmAbi for EntityId {
  type Abi = u64;

  fn into_abi(self) -> Self::Abi {
    self.0 .0
  }
}

impl FromWasmAbi for EntityId {
  type Abi = u64;

  unsafe fn from_abi(js: Self::Abi) -> Self {
    Self(EntityId(js))
  }
}

This is neat because you can make a pub field in your Rust struct that has type EntityId, but it'll be exposed as a nice simple bigint. No need for a class definition and garbage collectible object, etc.

However, experienced TypeScript developers like myself often want strong typing for their id types. We do things like:

export type Brand<T, Brand extends string> = T & {
  readonly [B in Brand as `__${B}_brand`]: never;
};

export type EntityId = Brand<bigint, "entity">;

If the .d.ts file were patched so that bigint is replaced with EntityId, it would still work just fine; it's actually the same ABI. However it would have the benefits of strong typing.

Proposed Solution

  • Add typescript_type: Option<String> to StructField in backend/src/ast.rs
  • Populate the above field in parser.rs by looking at attrs.typescript_type() (which already exists)
  • Add typescript_override: Option<&'a str> to StructField in shared/src/lib.rs
  • Populate the above field in shared_struct_field in encode.rs
  • Add a type_override field to AuxExport in wit/nonstandard.rs
  • Populate the above field in struct_ in wit/mod.rs
  • cli-support/src/js/mod.rs looks at export.type_override and if it's Some, it will replace ts_ret_ty

I would be happy to make a PR with the above changes.

@Liamolucko
Copy link
Collaborator

I think it might be better to put the annotation on the type (i.e. EntityId) rather than the field - with the current proposal, it wouldn't be possible to override the type in a scenario like this:

#[wasm_bindgen]
impl Foo {
    #[wasm_bindgen(getter)]
    pub fn bar(&self) -> EntityId { EntityId(5) }
}

You should be wary of manually using IntoWasmAbi / FromWasmAbi / WasmDescribe like that, though. As the docs for wasm_bindgen::convert say:

This is mostly an internal module, no stability guarantees are provided. Use at your own risk.

@prideout
Copy link
Author

Ah you're proposing adding it to a type that is not extern "C"? Yes I think that could work! In fact maybe it's already supported? I can try the experiment later tonight.

@prideout
Copy link
Author

The problem with using this attribute on my type definition is that a wrapper class is generated, which I'm trying to avoid. The situation mentioned by @Liamolucko can be handled by creating a new extern "C" type with a different name, e.g. JsEntityId, and returning that from a Rust function. However that isn't very ergonomic because I'd like my function to have both Rust clients and JS clients.

I just discovered that my desire to avoid wrapper classes for simple newtypes is an existing issue: #1474.

In an ideal world, we could have #1474 and use typescript_type on it, in which case I agree with @Liamolucko that I would have no need for it on struct fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants