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

Fixed generated types for getters and setters #4202

Merged
merged 6 commits into from
Oct 29, 2024

Conversation

RunDevelopment
Copy link
Contributor

While working on #4201, I had the idea to test how wasm bindgen handles getter-setter pairs with different types. Not well was the answer.

This PR completely changes how the type definitions for getters and setters are generated and fixes several bugs:

  1. Static getters/setters with the same name as instance getters/setters would interfere with each other and only result in one generated property. Whether this property was static depended on the order in which the getters/setters were declared in Rust.
  2. Getters and setters with different types only produced one property with one of the types. Which type the property had depended on the order in which the getter and setter were declared in Rust.
  3. (minor bug) Write-only properties (setter but no getter) produced incorrect type definition that allowed reading the property.

There were also some minor issues:

  • Fields were sorted by name and not written in the order in which they were declared like everything else.
  • Doc comments weren't indented properly.

This PR fixes all of those issues and adds a test to verify correctness.

Fixing these issues is a requirement for #4188 and #4201.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Just some nits.

CHANGELOG.md Outdated Show resolved Hide resolved
crates/cli-support/src/js/mod.rs Show resolved Hide resolved
Co-authored-by: daxpedda <[email protected]>
@daxpedda daxpedda added the needs review Needs a review by a maintainer. label Oct 15, 2024
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

LGTM, but I will give it another review with a fresh set of eyes tomorrow.
Thank you, this is great work!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: daxpedda <[email protected]>
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Apologies for the delay, was sick and had to play catch-up with my other projects before getting back to this again.

LGTM!

@daxpedda daxpedda merged commit 2f2ac1a into rustwasm:main Oct 29, 2024
41 checks passed
@RunDevelopment RunDevelopment deleted the fix-class-getter-setter branch October 29, 2024 10:18
@RunDevelopment
Copy link
Contributor Author

No worries. I'm glad you're feeling better!

And thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Needs a review by a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants