-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
fix: Decorators when super()
exists and protoInit
is not needed
#16385
Conversation
liuxingbaoyu
commented
Mar 25, 2024
Q | A |
---|---|
Fixed Issues? | Fixes #16383 |
Patch: Bug Fix? | √ |
Major: Breaking Change? | |
Minor: New Feature? | |
Tests Added + Pass? | √ |
Documentation PR Link | |
Any Dependency Changes? | |
License | MIT |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56579 |
4d53402
to
4b05c2c
Compare
} | ||
} | ||
|
||
const r = new TestChild(); |
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.
In contrast with initProto-existing-derived-constructor
, can you rename this test to initField-existing-derived-constructor
?
I suggest we also add some behaviour assertions like we have done in that test.
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.
I tried porting all the tests but unfortunately couldn't get them all to work so I just added a simple test.🤦♂️
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.
Can you push the new tests? Even if they are failing, we can still investigate for sure.
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.
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.
I re-added all the tests, but I'm not sure if I tested the behavior well, thanks for the review!
f182cbd
to
755ae4e
Compare
25c5235
to
4f1ee84
Compare
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. The new tests look go 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.
Looks good 👍