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

Fix duplicated assertThisInitialized calls in constructors #9458

Conversation

rubennorte
Copy link
Contributor

Q                       A
Fixed Issues? Fixes #8335
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This removes duplicated (and redundant) calls to the assertThisInitialized helper in constructors for subclasses in loose mode.

I've moved the detection of this to a later point in the transformation of the class to find the ones that are also added by super. calls, and then replaced them all at the same place (removing the redundancy in some cases).

@rubennorte rubennorte force-pushed the fix-duplicated-assert-this-initialized branch from 9a9e428 to 945436f Compare February 5, 2019 15:31
@babel-bot
Copy link
Collaborator

babel-bot commented Feb 5, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9997/

@rubennorte rubennorte force-pushed the fix-duplicated-assert-this-initialized branch from 945436f to 894d047 Compare February 6, 2019 13:40
@rubennorte rubennorte force-pushed the fix-duplicated-assert-this-initialized branch from 894d047 to 9c1e3a0 Compare February 6, 2019 14:31
@danez danez added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Feb 6, 2019
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.

LGTM! While testing this PR, I found another bug:

class Bar {}

class Foo extends Bar {
  constructor() {
    0 && super();
    1 || super();
  }
}

new Foo // It should throw, but doesn't.

After that this PR is merged, would you like to fix it?

(This bug isn't introduced by your PR, it was already there).

@nicolo-ribaudo
Copy link
Member

Thank you!

@nicolo-ribaudo nicolo-ribaudo merged commit 045d019 into babel:master Feb 7, 2019
@nicolo-ribaudo nicolo-ribaudo added PR: Polish 💅 A type of pull request used for our changelog categories Spec: Classes and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Feb 7, 2019
@rubennorte rubennorte deleted the fix-duplicated-assert-this-initialized branch February 7, 2019 23:45
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories Spec: Classes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redundant call to assertThisInitialized for classes in loose mode
5 participants