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

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

Conversation

dragomirtitian
Copy link
Contributor

Fixes #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."

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 5, 2021
@dragomirtitian dragomirtitian marked this pull request as ready for review February 5, 2021 16:11
@sandersn sandersn added this to Not started in PR Backlog Feb 16, 2021
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I think it's missing a check that the property is static.

src/compiler/diagnosticMessages.json Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
PR Backlog automation moved this from Not started to Waiting on author Mar 9, 2021
@dragomirtitian
Copy link
Contributor Author

@sandersn Thank you for the review. I fixed the issues. Let me know if there are any others changes I should make.

PR Backlog automation moved this from Waiting on author to Needs merge Apr 2, 2021
@sandersn
Copy link
Member

sandersn commented Apr 2, 2021

@dragomirtitian Sorry for the delay on this. I tried merging from master but there are some changes in the emit that were so complex that I couldn't tell whether they were correct. Can you take a look? I'll merge after that.

@dragomirtitian dragomirtitian force-pushed the GH-41788-incorrect-output-for-esprivate-with-nested-class-in-esnext branch from 90e6067 to dc0a347 Compare April 5, 2021 17:06
@sandersn sandersn merged commit 2484210 into microsoft:master Apr 7, 2021
PR Backlog automation moved this from Needs merge to Done Apr 7, 2021
@ExE-Boss
Copy link
Contributor

This also fixed #34787

sodatea added a commit to sodatea/vite that referenced this pull request Jul 14, 2021
Since TypeScript 4.3, `target: "esnext"` indicates that
`useDefineForClassFields: true` as the new default.
See <microsoft/TypeScript#42663>

So I'm explicitly adding this field to the tsconfigs to avoid any
confusions.

Note that `lit-element` projects must use
`useDefineForClassFields: false` because of <https://github.com/lit/lit-element/issues/1030>

Vue projects must use `useDefineForClassFields: true` so as to support
class style `prop` definition in `vue-class-component`:
<vuejs/vue-class-component#465>

Popular React state management library MobX requires it to be `true`:
<https://mobx.js.org/installation.html#use-spec-compliant-transpilation-for-class-properties>

Other frameworks seem to have no particular opinion on this.

So I turned it on in all templates except for the `lit-element` one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Incorrect output for #private with nested class in ESNext
5 participants