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 support for traits #2871

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Add support for traits #2871

wants to merge 13 commits into from

Conversation

ImUrX
Copy link

@ImUrX ImUrX commented Apr 20, 2022

Tries to fix #2073
I honestly don't know what I'm doing but I really want this and it sure will be a nice addition to the ecosystem.

  • Correctly implement information struct of traits on the AST
  • Extract information of trait with the proc-macro
  • Generate typescript code for traits so you don't have a bad time developing
  • Generate JS glue code correctly
  • Make test unit

@ImUrX
Copy link
Author

ImUrX commented Apr 24, 2022

@alexcrichton sorry to bother but is there a way to generate JSDocs and ts signatures with the adapter types. I think I need to read a little more to understand how adapter type works

or maybe im going the wrong way

@ImUrX
Copy link
Author

ImUrX commented Apr 25, 2022

Kinda related btw microsoft/TypeScript#10570

@ImUrX
Copy link
Author

ImUrX commented Jul 31, 2022

@alexcrichton sorry to bother but is there a way to generate JSDocs and ts signatures with the adapter types. I think I need to read a little more to understand how adapter type works

or maybe im going the wrong way

yeah this seems more complicated than I thought, is there no way to generate ts documentation without having code on the wit module? It really seems like the only way which means I need to modify the library more than I would like for generating the interface method signatures documentation...
@alexcrichton sorry for still bothering you, but what are your thoughts on how I should follow on this?

@alexcrichton
Copy link
Contributor

Sorry this is a feature which off the top of my head I don't really know how it would work or how it would be integrated. I unfortunately don't have the time to help out right now with deisgn as well.

@ImUrX
Copy link
Author

ImUrX commented Aug 4, 2022

yeah understandable, this is currently a mess but it was quite interesting to do.
I'm gonna finish the implementation and then clean some code and make all needed tests and see what needs fixing

/// The doc comments on this trait, if provided
pub comments: Vec<String>,
/// Whether to generate a typescript definition for this trait
pub generate_typescript: bool,

Choose a reason for hiding this comment

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

is there any point to have this field? traits/interfaces only exist in TS

Copy link
Author

Choose a reason for hiding this comment

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

Sorry i never answered this. You can use traits without typescript definitions. The proposed way of doing this was making the interface contain Symbols for each method. So when you want to run the function of a class implementing such trait you do

struct[Trait.someMethodName]()

This is still the same in Typescript, because when you add a method of the same name on a class on Typescript, you override the method definition of the interface. Which is not normal behavior on rust

@stan-irl
Copy link

this would honestly be awesome to have!!

Theres no way to guarantee API stability between TS/JS and rust right now because you cant have build time failure for JS type incompatibility. Its also really difficult to cover with tests because you cant force rust behaviour from TS/JS where the tests will be. Its very difficult to ensure that these codepaths get hit.

I cant believe this isn't a higher priority - how are teams working with this? Do they just deploy and wait for it to break?

@felipellrocha
Copy link

@alexcrichton Any changes on this? I could really use the feature on our production system. Currently we have a huge, nasty workaround that I would love to get rid of.

@blockiosaurus
Copy link

Well my attempts to merge in 0.2.81 to this branch were unsuccessful, because I also don't know what I'm doing. Getting failures in the CLI on decode.

@davidedellagiustina
Copy link

Quick question. Let's say I have a piece of code like the following:

#[wasm_bindgen]
pub struct A {}

#[wasm_bindgen]
impl A {
    pub fn a(&self) {
        // Do something
    }
}

pub trait Trait {
    fn b(&self);
}

impl Trait for A {
    fn b(&self) {
        // Do something else
    }
}

As far as I understand, this will only export a class A and its method a() in JavaScript, but I'd also like to call A::b() – is there a way to do this as of today?

@ImUrX
Copy link
Author

ImUrX commented Mar 22, 2023

provide a wrapper method for calling it :P

@davidedellagiustina
Copy link

@ImUrX ok, so that's the only solution for now. Thank you!

@daxpedda daxpedda added the needs discussion Requires further discussion label May 11, 2023
@daxpedda
Copy link
Collaborator

I would be happy to review the Rust and JS part, but the TS part needs a different maintainer that has the time to do the design work and eventually review it.

@ImUrX
Copy link
Author

ImUrX commented May 11, 2023

tbh the typescript part is easy, i just havent had the time and will to finish the PR, aside from the rust part of generating the typescript code being a lot of pain

@vdagonneau
Copy link

Hi, I'm interested in this feature, would you be ok if I took a stab at it using your PR as a base? I hope I'll be able to fix the last couple things.

@ImUrX
Copy link
Author

ImUrX commented Nov 6, 2023

yeah, be free

@pr1metine
Copy link

Hey, I'm also interested in this feature. Do you, @vdagonneau and everyone else, wanna team up and work on this PR?

@haraldreingruber-dedalus
Copy link
Contributor

This might also be related to this discussion: #3254

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

Successfully merging this pull request may close these issues.

Proposal: Use Symbol for trait impls
10 participants