Skip to content

Gh 41788 incorrect output for esprivate with nested class in esnext #49

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

Conversation

dragomirtitian
Copy link

Fixes microsoft#41788

Fixed as described in comment

  1. If target:esnext,then useDefineForClassFields: true will now be the default.
  2. If target:esnext, useDefineForClassFields: true we emit correct code so no further action is needed (ex)
  3. If target:esnext, useDefineForClassFields: false and we encounter a usage of a #private field in a static member we emit an error like "You can't do this, change useDefineForClassFields: true, or target an earlier version of ES."

@dragomirtitian dragomirtitian marked this pull request as draft February 5, 2021 09:50
Copy link

@mkubilayk mkubilayk left a comment

Choose a reason for hiding this comment

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

Looks good - one minor comment.

@@ -26349,6 +26350,31 @@ namespace ts {
let prop: Symbol | undefined;
if (isPrivateIdentifier(right)) {
const lexicallyScopedSymbol = lookupSymbolForPrivateIdentifierDeclaration(right.escapedText, right);

if(lexicallyScopedSymbol && (compilerOptions.target === ScriptTarget.ESNext && compilerOptions.useDefineForClassFields === false)) {

Choose a reason for hiding this comment

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

I think useDefineForClassFields === false would be fine here. Is there a particular reason to use the raw value instead?

Choose a reason for hiding this comment

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

Surprised to see there is no lint rule or auto-formatting for if(. 😄

Copy link
Author

Choose a reason for hiding this comment

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

@mkubilayk me too.. Will fix the formatting.
About useDefineForClassFields you are right using the defaulted value should be ok as well. I was thinking that if it's undefined the check should not kick in. But since if it's undefined it will default to true, it's ultimately the same thing, and being consistent is better.

}
}
}
}

Choose a reason for hiding this comment

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

Would be good to add some private methods and accessor cases to this test either here on in 42458, depending on which one goes in first.

Copy link
Author

Choose a reason for hiding this comment

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

I expect this to go in first. (FYI we will have conflict with private methods and accessors since this adds some extra messages)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants