-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Fixing delay caused in vscode due to pasteEdits #59542
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
Changes from 10 commits
10be994
0d5380b
7b1d203
b56a4af
4544ad3
44626c4
e12d2cf
f292acd
30e5e2d
8248ff8
c0b1356
b660771
c0f61de
9acc51a
aebaae1
5044795
76897ba
d7083b8
a29d5a8
f80483c
65490bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,12 +3,15 @@ import { | |
codefix, | ||
Debug, | ||
fileShouldUseJavaScriptRequire, | ||
findAncestor, | ||
findIndex, | ||
forEachChild, | ||
formatting, | ||
getQuotePreference, | ||
getTokenAtPosition, | ||
isIdentifier, | ||
Program, | ||
rangeContainsPosition, | ||
SourceFile, | ||
Statement, | ||
SymbolFlags, | ||
|
@@ -62,7 +65,6 @@ function pasteEdits( | |
} | ||
|
||
const statements: Statement[] = []; | ||
|
||
let newText = targetFile.text; | ||
for (let i = pasteLocations.length - 1; i >= 0; i--) { | ||
const { pos, end } = pasteLocations[i]; | ||
|
@@ -104,12 +106,33 @@ function pasteEdits( | |
preferences, | ||
formatContext, | ||
}; | ||
forEachChild(updatedFile, function cb(node) { | ||
if (isIdentifier(node) && !originalProgram?.getTypeChecker().resolveName(node.text, node, SymbolFlags.All, /*excludeGlobals*/ false)) { | ||
// generate imports | ||
importAdder.addImportForUnresolvedIdentifier(context, node, /*useAutoImportProvider*/ true); | ||
} | ||
node.forEachChild(cb); | ||
|
||
// `updatedRanges` represent the new ranges that account for the offset changes caused by pasting new text and | ||
// `offset` represents by how much the starting position of `pasteLocations` needs to be changed. | ||
// | ||
// We iterate over each updated range to get the node that wholly encloses the updated range. For each child of that node, it is checked if the | ||
// identifier lies within the updated range and if it is not resolved, we try resolving it. | ||
|
||
DanielRosenwasser marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const updatedRanges: TextRange[] = []; | ||
let offset = 0; | ||
pasteLocations.forEach((location, i) => { | ||
const deletionNeeded = location.pos === location.end ? 0 : location.end - location.pos; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'd recommend getting rid of the unnecessary branch and rewriting this to something like const selectedTextLength = location.end - location.pos; or const pasteSelectionLength = location.end - location.pos; or const oldTextLength = location.end - location.pos; |
||
const textToBePasted = actualPastedText ? actualPastedText[0] : pastedText[i]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it ever possible that the count of paste locations exceeds the number of items in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done at top of the function to make sure its always right There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, in that case all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha - while you're here, can you fix that so it uses the appropriate newline character instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean using |
||
const startPos = location.pos - offset; | ||
const endPos = startPos + textToBePasted.length; | ||
updatedRanges.push({ pos: startPos, end: endPos }); | ||
offset += deletionNeeded - textToBePasted.length; | ||
DanielRosenwasser marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
|
||
updatedRanges.forEach(range => { | ||
const enclosingNode = findAncestor(getTokenAtPosition(context.sourceFile, range.pos), ancestorNode => rangeContainsPosition(ancestorNode, range.end)); | ||
DanielRosenwasser marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aside: I almost wanted to suggest |
||
if (!enclosingNode) return; | ||
forEachChild(enclosingNode, function cb(node) { | ||
DanielRosenwasser marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (isIdentifier(node) && rangeContainsPosition(range, node.getStart(updatedFile)) && !originalProgram?.getTypeChecker().resolveName(node.text, node, SymbolFlags.All, /*excludeGlobals*/ false)) { | ||
importAdder.addImportForUnresolvedIdentifier(context, node, /*useAutoImportProvider*/ true); | ||
} | ||
node.forEachChild(cb); | ||
DanielRosenwasser marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
}); | ||
} | ||
importAdder.writeFixes(changes, getQuotePreference(copiedFrom ? copiedFrom.file : targetFile, preferences)); | ||
|
Uh oh!
There was an error while loading. Please reload this page.