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

Jump to bottom

Fix rewriteThis in helper-module-transforms for computed class elements #11109

Merged
merged 13 commits into from Feb 12, 2020
Merged

Fix rewriteThis in helper-module-transforms for computed class elements #11109

merged 13 commits into from Feb 12, 2020

Conversation

sidntrivedi012
Copy link
Contributor

@sidntrivedi012 sidntrivedi012 commented Feb 7, 2020 •
edited

Q                       A
Fixed Issues? Fixes #10784
Patch: Bug Fix? 👍
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Not as of now
Documentation PR Link
Any Dependency Changes?
License MIT

@JLHwung please review if I correctly used ClassMethod to find if its computed or not ? Also, where do we have to requeue the key child of ClassProperty to ?

@sidntrivedi012 sidntrivedi012 marked this pull request as ready for review February 7, 2020 22:44
@sidntrivedi012
Copy link
Contributor Author

sidntrivedi012 commented Feb 8, 2020 •
edited

The transformed code now throws can not read property name of undefinedas it should.

Screenshot 2020-02-08 at 8 03 42 PM

@nicolo-ribaudo nicolo-ribaudo added PR: Spec Compliance 👓 A type of pull request used for our changelog categories area: modules labels Feb 9, 2020
@sidntrivedi012
Copy link
Contributor Author

sidntrivedi012 commented Feb 10, 2020 •
edited

We have a method to skip everything but the computed key somewhere (it's used by environmentVisitor). We can re-use it here.

@nicolo-ribaudo Have used skipAllButComputedKey function here and for using that, imported the helper-replace-supers package. But still, the tests don't show the intended error 🤔 . Also, there's void 0 popping up instead of this in test output file 😅.PTAL. Thanks.

/cc: @JLHwung

nicolo-ribaudo
nicolo-ribaudo previously requested changes Feb 11, 2020
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a 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; }]() {}
}

packages/babel-helper-module-transforms/src/rewrite-this.js Show resolved Hide resolved
@nicolo-ribaudo nicolo-ribaudo dismissed their stale review February 11, 2020 11:20

You made the changes 30 seconds before my review 👍

nicolo-ribaudo
nicolo-ribaudo approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks!

@sidntrivedi012 @nicolo-ribaudo
Update packages/babel-helper-replace-supers/src/index.js
9867763 
Co-Authored-By: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
JLHwung
JLHwung approved these changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Good job!

existentialism
existentialism approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@existentialism existentialism left a 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?

@sidntrivedi012
Copy link
Contributor Author

👍

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 ?

@nicolo-ribaudo nicolo-ribaudo merged commit 3c6a8ae into babel:master Feb 12, 2020
rajasekarm pushed a commit to rajasekarm/babel that referenced this pull request Feb 17, 2020
@sidntrivedi012 @nicolo-ribaudo @rajasekarm
Fix rewriteThis in helper-module-transforms for computed class… (babe…
ecfd416 
…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>
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 15, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@existentialism existentialism existentialism approved these changes

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
area: modules outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Spec Compliance 👓 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rewriteThis in helper-module-transforms misses computed class elements
4 participants
@sidntrivedi012 @existentialism @JLHwung @nicolo-ribaudo