Avoid repeating codefix work when resolving auto-import specifiers for completions #49442
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
FYI @OliverJAsh—this was discovered in your repo 🙂. Thanks! |
| /** | ||
| * Computes module specifiers for multiple import additions to a file. | ||
| */ | ||
| export interface ImportSpecifierResolver { |
There was a problem hiding this comment.
I feel like I'm missing something obvious, but why not make this a function type?
There was a problem hiding this comment.
I really thought it was going to contain more than one method.
There was a problem hiding this comment.
To be fair in some languages they're the same. Just not in our language. :D
There was a problem hiding this comment.
So, I thought about moving more (all?) of the exported functions into here so that other things that want to call importFixes.ts logic don’t make the same mistake. E.g., I’m pretty sure the same inefficiency happens with the “add missing properties/methods” codefix when it needs to import more than one type to write out signatures. I decided against a bigger refactoring since we were considering this for a patch, but in the future I would like this object to hold all the importing-file-specific info involved with computing module specifiers that could get reused between resolving multiple module specifiers. That will also help with the growing issue in this file that every function takes about a dozen parameters that just get fed through from the top. It will be much easier to read these function signatures if we just close over importingFile, program, host, preferences, formatContext, packageJsonImportFilter, exportInfoMap, .... So that’s why I don’t want to replace this with createGetModuleSpecifierForBestExportInfo that just returns the one function.
|
@typescript-bot cherry-pick this to release-4.7 |
|
Heya @andrewbranch, I've started to run the task to cherry-pick this into |
| if (!(targetFlags & SymbolFlags.Value) && isSourceFileJS(importingFile)) return emptyArray; | ||
| const importKind = getImportKind(importingFile, exportKind, compilerOptions); | ||
| return mapDefined(importingFile.imports, (moduleSpecifier): FixAddToExistingImportInfo | undefined => { | ||
| function createExistingImportMap(checker: TypeChecker, importingFile: SourceFile, compilerOptions: CompilerOptions) { |
There was a problem hiding this comment.
Something about the name of this feels off - what does existing refer to?
There was a problem hiding this comment.
Import declarations that already exist in the file
There was a problem hiding this comment.
createImportMapForExistingExports then probably
There was a problem hiding this comment.
Huh? It’s not exports that are existing, it’s the imports. See historical usage of ExistingImports all over this file 😄
There was a problem hiding this comment.
The purpose of this module is to add imports to a file. By definition, those imports do not exist yet. Contrast them with imports that do exist in the file, which can be used in various ways in the process of adding new ones. Those are what this map contains. It’s a map of imports that exist in the file. We need to specify that they exist because so much in the module deals with hypothetical imports that we might choose to write into existence. Everywhere an existing import is mentioned, it’s called an “existing import,” e.g. the type FixAddToExistingImport (a type of fix that adds a new specifier to an existing import declaration). So, I think it’s fair and consistent to say this is a map of existing imports. (More specifically, the keys are the symbol ids of other modules, and the values are the local import declarations that resolve to those modules.)
Component commits: d8f251b Avoid repeating codefix work when resolving auto-import specifiers for completions
|
Hey @andrewbranch, I've opened #49447 for you. |
Component commits: d8f251b Avoid repeating codefix work when resolving auto-import specifiers for completions Co-authored-by: Andrew Branch <andrew@wheream.io>
Fixes an auto-imports perf regression from 4.6 to 4.7 (and improves the perf over 4.6). The following measurements (in ms) were pulled from the
collectAutoImportstime reported in TS Server logs in a very large project (~18k auto imports):moduleResolution- scenarionodenext- first triggernodenext- second triggernode- first triggernode- second triggerIn
nodenext, all module specifiers are resolved and cached during the first trigger, which is why the difference between nightly and this PR is so big on the second trigger—most of the work being done was unnecessary repeated work. Innode, both the first and second triggers resolve module specifiers (and much fewer of them than the first trigger innodenextdoes), so the difference is a bit smaller. All of these measurements represent only a portion of total completions time, but in each of the “second trigger” scenarios,collectAutoImportsis a significant portion of that total time.