-
-
Notifications
You must be signed in to change notification settings - Fork 411
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 a js_class to implement the Class trait without boilerplate #3872
Conversation
This also adds a JsInstance that verifies that `this` is of the proper class, and an `Ignore` that ignore arguments. The syntax is a bit special because of limitations of macro_rules. For example, the way fields are defined. It was impossible to keep both assignments (e.g. `public field = 123;`) and dynamic fields. This reduces significantly the boilerplate for declaring classes. The example in class.rs is more than twice as long (if you remove comments) than the macro is.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3872 +/- ##
==========================================
+ Coverage 47.24% 51.19% +3.94%
==========================================
Files 476 470 -6
Lines 46892 45401 -1491
==========================================
+ Hits 22154 23242 +1088
+ Misses 24738 22159 -2579 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really impressive work! Definitely having a boilerplate-free way to create JsClasses is a thing we should have.
I was wondering, what is the benefit of doing it with a macro_rules vs a procedural macro? I'm just worried about using macro_rules for this since it would pretty much disable all IDE autocompletions vs a procedural macro, which is a lot nicer to IDEs.
I was envisioning something like wasm_bindgen's getter
and setter
attributes
It was just easier for me to build a macro, and I wanted to have a more JS-ey way to describe the class. It would make sense to have a proc macro at some point, I just don't have the time yet. This reduces a lot of code already. |
Fail enough. I think we can start by offering a macro rules, then switch to a proc macro in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I have some suggestions that remove the unwrap
s.
@jedel1043 PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks good to me! Thanks for all your work on this!
This also adds a JsInstance that verifies that
this
is of the proper class, and anIgnore
that ignore arguments.The syntax is a bit special because of limitations of macro_rules. For example, the way fields are defined. It was impossible to keep both assignments (e.g.
public field = 123;
) and dynamic fields.This reduces significantly the boilerplate for declaring classes. The example in class.rs is more than twice as long (if you remove comments) than the macro is.