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.

Sign up for GitHub

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: class field is not accessible via super #54056

Merged
merged 3 commits into from Sep 22, 2023
Merged

fix: class field is not accessible via super #54056

merged 3 commits into from Sep 22, 2023

Conversation

Jack-Works
Copy link
Contributor

@Jack-Works Jack-Works commented Apr 28, 2023 •
edited

fixes #54054
fixes #35314
fixes #54900

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 28, 2023
@Jack-Works Jack-Works mentioned this pull request Apr 29, 2023
Closed
@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 29, 2023
@sandersn sandersn added this to Not started in PR Backlog May 8, 2023
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog May 9, 2023
@Guergeiro
Copy link

Just a question, does this handle the JS getter methods? Get methods work with super call in JS.

@Jack-Works
Copy link
Contributor Author

Just a question, does this handle the JS getter methods? Get methods work with super call in JS.

Only class fields are affected. getter and setters work normally.

@Jack-Works Jack-Works force-pushed the class-field-super branch 3 times, most recently from 8cd587c to 1a3b36b Compare June 10, 2023 11:30
PR Backlog automation moved this from Waiting on reviewers to Waiting on author Jun 12, 2023
@jackofsometrade jackofsometrade mentioned this pull request Jul 6, 2023
Closed
@ekilah
Copy link

ekilah commented Jul 6, 2023 •
edited

I was referred to this fix from my issue:

I believe:

fixes #54900

PR Backlog automation moved this from Waiting on author to Needs merge Jul 7, 2023
@andrewbranch
Copy link
Member

@typescript-bot run dt
@typescript-bot test top100

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 7, 2023 •
edited

Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 2dc49db. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 7, 2023 •
edited

Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 2dc49db. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@andrewbranch Here are the results of running the top-repos suite comparing main and refs/pull/54056/merge:

Something interesting changed - please have a look.

Details

appsmithorg/appsmith

7 of 9 projects failed to build with the old tsc and were ignored

app/client/packages/rts/tsconfig.json

microsoft/vscode

5 of 53 projects failed to build with the old tsc and were ignored

src/tsconfig.tsec.json

@typescript-bot
Copy link
Collaborator

Hey @andrewbranch, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@andrewbranch
Copy link
Member

These errors might be interesting to look at.

@andrewbranch
Copy link
Member

In appsmith, the code presumably works at runtime because the compile target is es6 where class fields aren’t supported. VS Code uses useDefineForClassFields: false which I assume has the same effect for this context. We could choose to only issue this error when class fields will be untransformed, but (1) the code is wrong even if it will technically work, and (2) we don’t know anything about the emit of dependency code, so it would be better to do this consistently. Since we’re past 5.2-beta, I think we should wait until 5.3 to merge this breaking change.

@andrewbranch andrewbranch added the Breaking Change Would introduce errors in existing code label Jul 7, 2023
@Jack-Works
Copy link
Contributor Author

In appsmith, the code presumably works at runtime because the compile target is es6 where class fields aren’t supported. VS Code uses useDefineForClassFields: false which I assume has the same effect for this context. We could choose to only issue this error when class fields will be untransformed, but (1) the code is wrong even if it will technically work, and (2) we don’t know anything about the emit of dependency code, so it would be better to do this consistently. Since we’re past 5.2-beta, I think we should wait until 5.3 to merge this breaking change.

Sorry, I'm confused, in what case it can be valid? I cannot think of a setup that defines a class field on the prototype instead of the instance.

@andrewbranch
Copy link
Member

Hm, I assumed that a quirk of the transformation down to ES2015 would make it work somehow, but I think you’re right. Maybe this stuff isn’t working at all? This is very odd 🤔

@sandersn
Copy link
Member

Even stranger, the first error, in appsmith, looks like a typo.

sandersn
sandersn approved these changes Jul 14, 2023
View reviewed changes
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 agree that this should go into 5.3 to avoid breaking vscode after the 5.2 beta.

In the meantime, I had one small nit about the test, but I'm not 100% what a better solution is.

tests/cases/compiler/classFieldSuperNotAccessible.ts Outdated Show resolved Hide resolved
@sandersn sandersn merged commit 5f818a9 into microsoft:main Sep 22, 2023
19 checks passed
PR Backlog automation moved this from Needs merge to Done Sep 22, 2023
@Jack-Works Jack-Works deleted the class-field-super branch September 23, 2023 06:55
snovader pushed a commit to mestro-se/TypeScript that referenced this pull request Sep 23, 2023
@Jack-Works @sandersn @snovader
fix: class field is not accessible via super (microsoft#54056)
4c9bccf 
Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>
@andrewbranch andrewbranch mentioned this pull request Sep 26, 2023
Open
This was referenced Sep 26, 2023
Merged
Closed
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.3.0 milestone Sep 26, 2023
@Andarist Andarist mentioned this pull request Sep 27, 2023
Merged
@DanielRosenwasser DanielRosenwasser mentioned this pull request Sep 28, 2023
Open
@LouieK22 LouieK22 mentioned this pull request Feb 14, 2024
Merged
osyrisrblx added a commit to roblox-ts/roblox-ts that referenced this pull request Feb 14, 2024
@LouieK22 @osyrisrblx
TypeScript 5.3 (#2617)
2c422eb 
- Bumps `typescript` and `ts-expose-internals` to v5.3.3
- Removes the test diagnostic for `noSuperProperty` as the compiler
catches this now (microsoft/TypeScript/pull/54056)
- Make 1:1 changes for methods and fields
- Handles the `NoSubstitutionTemplateLiteral` node from
microsoft/TypeScript/pull/55930

Please take a look at the module resolution change. I'm not sure if this
will have downstream effects that I couldn't catch in testing.

Closes #2610

---------

Co-authored-by: Osyris <osyrisrblx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandersn sandersn sandersn approved these changes

@andrewbranch andrewbranch andrewbranch approved these changes

@navya9singh navya9singh Awaiting requested review from navya9singh

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

Assignees

@andrewbranch andrewbranch

Labels
Breaking Change Would introduce errors in existing code For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
  
Done
Milestone
TypeScript 5.3.0
7 participants
@Jack-Works @Guergeiro @ekilah @andrewbranch @typescript-bot @sandersn @DanielRosenwasser