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 rewriteThis in helper-module-transforms for computed class elements #11109
Fix rewriteThis in helper-module-transforms for computed class elements #11109
Conversation
4bf38d9
to
d47eba7
Compare
...lugin-transform-modules-commonjs/test/fixtures/rewritethis-computed-class-elements/input.mjs
Outdated
Show resolved
Hide resolved
@nicolo-ribaudo Have used /cc: @JLHwung |
...in-transform-modules-commonjs/test/fixtures/misc/undefined-this-computed-class-key/input.mjs
Outdated
Show resolved
Hide resolved
...in-transform-modules-commonjs/test/fixtures/misc/undefined-this-computed-class-key/input.mjs
Outdated
Show resolved
Hide resolved
...in-transform-modules-commonjs/test/fixtures/misc/undefined-this-computed-class-key/output.js
Outdated
Show resolved
Hide resolved
…computed key for methods
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.
Could you also add these tests for more complex cases?
class A {
[() => this.name]() {}
}
class A {
[function () { this.name; }]() {}
}
You made the changes 30 seconds before my review 👍
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.
Thanks!
Co-Authored-By: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
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.
Good job!
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.
👍
Sidenote: we may want to consider where skipAllButComputedKey
lives (separate from this PR of course), as it seems weird to need to import replace supers for it?
Sure, should I open a discussion issue for it if needed ? |
…l#11109) * Fix rewriteThis in helper-module-transforms for computed class elements * Added test file and corrected the visitor * Revert .gitignore * Using skipAllButComputedKey method from plugin-replace-supers * added tests for class methods * Added tests for both class properties and methods and fixed skipping computed key for methods * Fixed condition for class methods * revised the conditions for class methods * Added more tests and used else-if in classmethod condition * Update packages/babel-helper-replace-supers/src/index.js Co-Authored-By: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
@JLHwung please review if I correctly used
ClassMethod
to find if its computed or not ? Also, where do we have to requeue thekey
child ofClassProperty
to ?