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

Add typescript_type attribute #1986

Merged
merged 2 commits into from
Mar 13, 2020
Merged

Add typescript_type attribute #1986

merged 2 commits into from
Mar 13, 2020

Conversation

clearloop
Copy link
Contributor

proc-macro attribute

#[wasn_bindgen(plain_object)]

Motivation

Ref to @Pauan 's suggestion in #1591, I just wrote an attribute named plain_object.

I am writing a wasm UI library these days, and I have to duplicate some types for 3 times(in my project, rust for 2 and ts for 1) without the ability exporting typed struct from rust...

I feel very excited about wasm, and never thinking about that I can contribute to wasm-bindgen one day, hope this pr can help.

Solution

  1. Add an attribute named plain_object
  2. Not generating constructor with palin_object
  3. Hide free() void; while generating .d.ts

Result

Now we can export rust types like:

#[wasm_bindgen(plain_object)]
pub struct Ping {
    pong: bool
}

And in typescript file:

import * as r from "pkg";

const p: r.Ping = {
    pong: true,
};

Confusions

I'm quite a newbie in programming, not sure about if just hide free() void; in d.ts is okay.

@clearloop
Copy link
Contributor Author

This pr has some problems, plain_object can pass the type check, but can not generate the target struct, and...add 61 lines to delete 1 line, it's kind of stupid, I'll try to find a better way.

@alexcrichton
Copy link
Contributor

Thanks for the PR! I'm not personally too familiar with the motivations here, but @Pauan would you be able to review this?

@clearloop
Copy link
Contributor Author

clearloop commented Feb 5, 2020

What about adding an object_args attribute to use with typescript_custom_section?

I just got a new idea to deal this with JsValue, for Example:

#[wasm_bindgen(typescript_custom_section)]
const ITEXT_STYLE: &'static str = r#"
interface ITextStyle  {
    bold: boolean;
    italic: boolean;
    size: i32;
}
"#;

#[wasm_bindgen]
struct TextStyle {
    pub bold: bool,
    pub italic: bool,
    pub size: i32,
}

#[wasm_bindgen]
impl TextStyle {
    #[wasm_bindgen(constructor, object_args="ITextStyle")]
    pub fn new(interface: JsValue) {
        // parse JsValue
    }
}

and in typescript, we can generate TextStyle like this:

import { TextStyle } from "../pkg";

const style = new TextStyle({
    bold: true,
    italic: true,
    size: 8,
});

wasm_bindgen is so much awesome that I'm keeping blowing up these days, it seems like that I just find a direction to do something for the web, for the meaning of my life.

@alexcrichton alexcrichton requested a review from Pauan February 6, 2020 18:27
@Pauan
Copy link
Contributor

Pauan commented Feb 7, 2020

Although I originally suggested this, I'm not sure plain_object is the right approach. The problem with plain_object is that we're not really sending a Rust struct to JS, instead we're dealing with a JS object.

So I agree that something like object_args would be better. However, we can make it a lot more flexible, like this:

#[wasm_bindgen(typescript_custom_section)]
const ITEXT_STYLE: &'static str = r#"
interface ITextStyle {
    bold: boolean;
    italic: boolean;
    size: i32;
}
"#;

#[wasm_bindgen]
extern "C" {
    #[wasm_bindgen(typescript_type = "ITextStyle")]
    pub type ITextStyle;
}

#[wasm_bindgen]
struct TextStyle {
    pub bold: bool,
    pub italic: bool,
    pub size: i32,
}

#[wasm_bindgen]
impl TextStyle {
    #[wasm_bindgen(constructor)]
    pub fn new(interface: ITextStyle) {
        // parse JsValue
    }
}

The typescript_type attribute allows you to attach a TypeScript type to the Rust type. Then this will be reflected in the generated .d.ts.

The benefit of typescript_type rather than object_args is that it will automatically work correctly in all places: struct fields, function arguments, and function returns.

The above code is rather verbose, however it's possible to create a macro which will generate it:

typescript! {
    interface ITextStyle  {
        bold: boolean;
        italic: boolean;
        size: i32;
    }
}

So I think it would be best to add in support for typescript_type, rather than plain_object or object_args.

@clearloop
Copy link
Contributor Author

Cool! I agree with typescript_type is the best, I'll try to implement this!

@Pauan
Copy link
Contributor

Pauan commented Mar 2, 2020

@clearloop As a consequence of #2012 I needed to add a typescript_type attribute (based on the already existing typescript_name attribute). I did some basic tests, and it seems to work correctly.

But this PR is still useful, since it adds in some unit tests. So could you please rebase this on master?

@clearloop
Copy link
Contributor Author

clearloop commented Mar 3, 2020

@Pauan Yep, I'll do it right now!

(Waiting for your reply for a long long time...)

@clearloop
Copy link
Contributor Author

clearloop commented Mar 3, 2020

It occurs lots of warnings in my fork, even before rebase

warning: conflicting implementations of trait `convert::traits::IntoWasmAbi` for type `&dyn for<'r> core::ops::Fn(&'r _) -> _`:
   --> src/convert/closures.rs:11:9
    |
11  | /         impl<'a, 'b, $($var,)* R> IntoWasmAbi for &'a (dyn Fn($($var),*) -> R + 'b)
12  | |             where $($var: FromWasmAbi,)*
13  | |                   R: ReturnWasmAbi
14  | |         {
...   |
22  | |             }
23  | |         }
    | |_________^ conflicting implementation for `&dyn for<'r> core::ops::Fn(&'r _) -> _`

I didn't see these before...should I try to fix these?

@clearloop
Copy link
Contributor Author

clearloop commented Mar 3, 2020

Waiting for your review @Pauan, I also have a big update for my project if typescript_type can merge into master~

Well, after l fix the tests...

@clearloop clearloop changed the title Add plain_object attribute Add typescript_type attribute Mar 3, 2020
@Pauan
Copy link
Contributor

Pauan commented Mar 4, 2020

Waiting for your reply for a long long time...

Sorry, I'm usually busy with a lot of different things, so things sometimes slip through the cracks. If I'm taking too long to respond please do ping me.

I also have a big update for my project if typescript_type can merge into master~

To be clear, #2012 already added in typescript_type, so the only thing this PR needs to do is add some unit tests, and change the documentation so that it says that all TypeScript types are supported.

@clearloop
Copy link
Contributor Author

Thanks for offering me this opportunity, I'll remove my verbose code(except the test) right now~

Copy link
Contributor

@Pauan Pauan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@Pauan Pauan merged commit 5acd6a3 into rustwasm:master Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants