Fix overlapping changes when un-qualifying import use site within transformed export #40987
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
| @@ -199,7 +199,26 @@ namespace ts.codefix { | |||
| } | |||
| }); | |||
|
|
|||
| return getSynthesizedDeepCloneWithRenames(nodeToRename, /*includeTrivia*/ true, identsToRenameMap, checker); | |||
| return getSynthesizedDeepCloneWithReplacements(nodeToRename, /*includeTrivia*/ true, original => { | |||
There was a problem hiding this comment.
This was, until now, the only consumer of getSynthesizedDeepCloneWithRenames, so I moved the bits that had poor reusability out to here.
There was a problem hiding this comment.
weird that it had an unused optional callback parameter
There was a problem hiding this comment.
It was used originally, but I stopped using it a few months back when I greatly simplified convertToAsyncFunction.
| @@ -199,7 +199,26 @@ namespace ts.codefix { | |||
| } | |||
| }); | |||
|
|
|||
| return getSynthesizedDeepCloneWithRenames(nodeToRename, /*includeTrivia*/ true, identsToRenameMap, checker); | |||
| return getSynthesizedDeepCloneWithReplacements(nodeToRename, /*includeTrivia*/ true, original => { | |||
There was a problem hiding this comment.
weird that it had an unused optional callback parameter
| @@ -115,7 +129,7 @@ namespace ts.codefix { | |||
| } | |||
| case SyntaxKind.BinaryExpression: { | |||
| const { operatorToken } = expression as BinaryExpression; | |||
| return operatorToken.kind === SyntaxKind.EqualsToken && convertAssignment(sourceFile, checker, expression as BinaryExpression, changes, exports); | |||
| return operatorToken.kind === SyntaxKind.EqualsToken && convertAssignment(sourceFile, checker, expression as BinaryExpression, changes, exports, useSitesToUnqualify); | |||
There was a problem hiding this comment.
is a change to convertAssignment already in this PR? Or did it already take a map of use sites to unqualify?
There was a problem hiding this comment.
It’s here, GitHub folded over it: https://github.com/microsoft/TypeScript/pull/40987/files#diff-d91b63cfe65f0adf550d61fede06a78dR213
| useSitesToUnqualify?: ESMap<Node, Node>; | ||
| } | ||
|
|
||
| function convertedImports(newImports: readonly Node[], useSitesToUnqualify?: ESMap<Node, Node>): ConvertedImports { |
There was a problem hiding this comment.
This function seems a bit superfluous to me; there's only one call with a second argument.
I guess it lets you avoid writing casts or explicit return types in some places?
There was a problem hiding this comment.
It was because of all the places that didn’t have the second argument that I added this; all those were one-liner returns that would have become four lines in order to add the object literal with useSitesToUnqualify: undefined on a line.
| @@ -200,7 +223,7 @@ namespace ts.codefix { | |||
| changes.delete(sourceFile, assignment.parent); | |||
| } | |||
| else { | |||
| const replacement = isObjectLiteralExpression(right) ? tryChangeModuleExportsObject(right) | |||
| const replacement = isObjectLiteralExpression(right) ? tryChangeModuleExportsObject(right, useSitesToUnqualify) | |||
There was a problem hiding this comment.
I don't know this code at all, but it seems like there are a lot of potential places that need to be threaded through with useSitesToUnqualify. Are you confident you didn't miss any?
There was a problem hiding this comment.
Yep, pretty confident. I had never seen this code before either, but I ended up reading pretty much all the parts that actually perform text changes.
Fixes #40983
Can’t believe we didn’t get this one before now 🤔