Merge 'decorators' into 'modifiers' on various nodes #49089
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rbuckton
requested review from
sandersn,
DanielRosenwasser,
weswigham,
andrewbranch,
RyanCavanaugh and
sheetalkamat
typescript-bot
added
Author: Team
For Uncommitted Bug
rbuckton
force-pushed
the
mergeDecoratorsAndModifiers
branch
from
ea48336
to
1cf55ef
Compare
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 1cf55ef. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:Comparison Report - main..49089
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at c7fce15. You can monitor the build here. Update: The results are in! |
rbuckton
force-pushed
the
mergeDecoratorsAndModifiers
branch
from
c7fce15
to
01f9749
Compare
|
@rbuckton Here they are:Comparison Report - main..49089
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Performance is fairly close. There's unfortunately a little bit more megamorphism due to Decorator and Modifier being in the same array, but the impact has been fairly minimal (less than a tenth of a second in total compile time). The upside is that memory usage dropped across the board, and is especially noticeable in xstate. |
# Conflicts: # src/compiler/checker.ts # src/compiler/transformers/ts.ts # src/compiler/utilities.ts
rbuckton
force-pushed
the
mergeDecoratorsAndModifiers
branch
from
0f71949
to
a906d6f
Compare
sandersn
approved these changes
Jun 6, 2022
rbuckton
force-pushed
the
mergeDecoratorsAndModifiers
branch
from
a906d6f
to
d6e4576
Compare
# Conflicts: # src/testRunner/unittests/tsbuild/helpers.ts
…tions are printed
|
Got slightly ahead of myself with the merge, but I've fixed the test failure in |
|
I discovered that this breaks e.g. tsutils as NodeArray isn't compatible with ModifiersArray which is expected here. I see that the plan is to make it a soft failure in 4.8, do you also have a suggestion on how to make the code compatible with both 4.7 and 4.8? |
|
Whilst investigating typescript-eslint/typescript-eslint#5496 I locally upgraded our codebase to v4.8.1-rc and then did not see any relevant TS errors - it wasn't until I manually inspected the code that I noticed the VSCode strike-out decoration on the deprecated members that I found out about the change. From the perspective of an AST consumer - deprecating the now dead properties instead of removing them entirely is a bit of a problem. We'll have to manually audit our entire codebase to find the code that's now broken. |
ttshivers
added a commit
to ttshivers/nest-cli
that referenced
this pull request
Apr 9, 2023
Update tsconfig-paths hook to be compatible with how `updateExportDeclaration` now works in typescript 5. This is from microsoft/TypeScript#49089
3 tasks
ttshivers
added a commit
to ttshivers/nest-cli
that referenced
this pull request
Apr 15, 2023
Update tsconfig-paths hook to be compatible with how `updateExportDeclaration` now works in typescript 5. This is from microsoft/TypeScript#49089
ttshivers
added a commit
to ttshivers/nest-cli
that referenced
this pull request
Apr 15, 2023
Update tsconfig-paths hook to be compatible with how `updateExportDeclaration` now works in typescript 5. This is from: microsoft/TypeScript#49089
ttshivers
added a commit
to ttshivers/nest-cli
that referenced
this pull request
Apr 15, 2023
Decorators are now placed on `modifiers` on TypeScript's syntax trees. Passing decorators separately has been deprecated since v4.8 and removed completely in v5. https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-8.html#decorators-are-placed-on-modifiers-on-typescripts-syntax-trees microsoft/TypeScript#49089
ttshivers
added a commit
to ttshivers/nest-cli
that referenced
this pull request
May 17, 2023
Decorators are now placed on `modifiers` on TypeScript's syntax trees. Passing decorators separately has been deprecated since v4.8 and removed completely in v5. https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-8.html#decorators-are-placed-on-modifiers-on-typescripts-syntax-trees microsoft/TypeScript#49089
ph-fritsche
added a commit
to ph-fritsche/cjstoesm
that referenced
this pull request
Dec 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This merges the
decoratorsfield onNodeinto themodifiersfield in preparation for eventual support for Decorators Stage 3. This is necessary since the version of the Decorators proposal that advanced parses class decorators after theexportanddefaultkeywords, which is incompatible with our current AST andNodeFactorymethods.To avoid a large amount of complex overloads, we've currently opted to change thedecoratorsparameter on variousNodeFactoryAPI methods to only acceptnull(which we generally don't use in the compiler). Calling the existingNodeFactoryAPI methods with a value fordecoratorsthat is notnullwill result in a deprecation warning, but should otherwise work for the time being. To avoid a major runtime breaking API change in 4.x releases, we likely won't remove thedecoratorsparameter entirely until 5.0.NOTE: After some consideration, I've amended the approach above to remove the
decoratorsparameter entirely from the main API and only support it through overloads via deprecatedCompat.This also reorganizes the deprecatedCompat project for easier maintenance, and moves a few deprecated APIs out of compiler to reduce overhead in tsc.js.
NOTE TO REVIEWERS: I've added a large number of reviewers since this pertains to the public API so I can get more eyes on this.