FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fixed multiple prologue directives with parameter properties by JoshuaKGoldberg · Pull Request #48687 · microsoft/TypeScript · GitHub

/ TypeScript Public

Fixed multiple prologue directives with parameter properties - #48687

Merged
Jake Bailey (jakebailey) merged 1 commit into
microsoft:mainfrom
JoshuaKGoldberg:fix-parameter-properties-and-prologue-directives
Apr 14, 2022
ConversationCommits (1)ChecksFiles changed
Merged

Fixed multiple prologue directives with parameter properties#48687
Jake Bailey (jakebailey) merged 1 commit into
microsoft:mainfrom
JoshuaKGoldberg:fix-parameter-properties-and-prologue-directives

Conversation

Josh Goldberg ✨ (JoshuaKGoldberg) commented Apr 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Two issues:

  • Parameter properties should be inserted after any prologue directives
  • Prologue directives were getting copied over twice when there wasn't a super()

Fixes #48671

TypeScript Bot (typescript-bot) added the For Milestone Bug PRs that fix a bug with a specific milestone label Apr 13, 2022
Josh Goldberg ✨ (JoshuaKGoldberg) commented Apr 13, 2022
View reviewed changes
Comment thread src/compiler/checker.ts
break;
}
if (!isPrologueDirective(statement) && nodeImmediatelyReferencesSuperOrThis(statement)) {
if (nodeImmediatelyReferencesSuperOrThis(statement)) {

Josh Goldberg ✨ (JoshuaKGoldberg) Apr 13, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This change isn't necessary for the PR; I just had only noticed the check is unnecessary after #29374 was merged 😄

Josh Goldberg ✨ (JoshuaKGoldberg) commented Apr 13, 2022
View reviewed changes
Comment thread src/compiler/transformers/ts.ts
const indexAfterLastPrologueStatement = factory.copyPrologue(body.statements, statements, /*ensureUseStrict*/ false, visitor);
const superStatementIndex = findSuperStatementIndex(body.statements, indexAfterLastPrologueStatement);
const prologueStatementCount = factory.copyPrologue(body.statements, statements, /*ensureUseStrict*/ false, visitor);
const superStatementIndex = findSuperStatementIndex(body.statements, prologueStatementCount);

Josh Goldberg ✨ (JoshuaKGoldberg) Apr 13, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm nitpicking the variable name here. Both variable names are correct; prologueStatementCount just makes more sense to me -- especially in the context of the new array spread below.

Josh Goldberg ✨ (JoshuaKGoldberg) marked this pull request as ready for review April 13, 2022 23:44
Jake Bailey (jakebailey) merged commit fa2515e into microsoft:main Apr 14, 2022

Jake Bailey (jakebailey) commented Apr 14, 2022

Copy link
Copy Markdown
Member

Thanks for fixing this!

Josh Goldberg ✨ (JoshuaKGoldberg) deleted the fix-parameter-properties-and-prologue-directives branch April 14, 2022 16:34
P Sz (p-szm) mentioned this pull request Apr 19, 2022
Closed
This was referenced Apr 19, 2022
Closed
Merged

Jake Bailey (jakebailey) commented Apr 20, 2022

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) cherry-pick this to release-4.6

TypeScript Bot (typescript-bot) commented Apr 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the task to cherry-pick this into release-4.6 on this PR at 76f482f. You can monitor the build here.

TypeScript Bot (typescript-bot) commented Apr 20, 2022

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.6 manually.

Jake Bailey (jakebailey) pushed a commit to jakebailey/TypeScript that referenced this pull request Apr 20, 2022
Fixed multiple prologue directives with parameter properties (microso…
fc3d63e 
…ft#48687)

Jake Bailey (jakebailey) commented Apr 20, 2022

Copy link
Copy Markdown
Member

Sent #48782 manually. This PR is needed for the other PRs thanks to this PR affecting the base ts.ts transform.

Jack Works (Jack-Works) pushed a commit to Jack-Works/TypeScript that referenced this pull request Apr 22, 2022
Fixed multiple prologue directives with parameter properties (microso…
ae6696f 
…ft#48687)
Merged
Daniel Rosenwasser (DanielRosenwasser) pushed a commit that referenced this pull request May 3, 2022
Fixed multiple prologue directives with parameter properties (#48687) (…
81f1e5f 
…#48782)

Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com>
Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
This file contains hidden or 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
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Jake Bailey (jakebailey) jakebailey approved these changes

Assignees

Jake Bailey (jakebailey)

Labels

For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

No-op string in constructor is repeated and reordered when constructor assignment is used

3 participants

Back | FazBrowse Home | New Git URL