-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Proposal: If there’s a package.json, only auto-import things in it, more or less #31893
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 all commits
40321ad
b43f088
7af4d37
ab0339c
a1a9b55
afa8461
e625471
921a1b7
97c00b6
4945c87
2d7eaf9
306afe6
488fa8f
98436c9
cb2eef9
7639059
d6767a8
7268bca
3962d4a
b2ce461
f276466
ecd15e5
ac0f70a
fbb05d5
de8ef32
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 |
---|---|---|
|
@@ -283,13 +283,25 @@ namespace ts.codefix { | |
preferences: UserPreferences, | ||
): ReadonlyArray<FixAddNewImport | FixUseImportType> { | ||
const isJs = isSourceFileJS(sourceFile); | ||
const { allowsImporting } = createLazyPackageJsonDependencyReader(sourceFile, host); | ||
const choicesForEachExportingModule = flatMap(moduleSymbols, ({ moduleSymbol, importKind, exportedSymbolIsTypeOnly }) => | ||
moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile, host, program.getSourceFiles(), preferences, program.redirectTargetsMap) | ||
.map((moduleSpecifier): FixAddNewImport | FixUseImportType => | ||
// `position` should only be undefined at a missing jsx namespace, in which case we shouldn't be looking for pure types. | ||
exportedSymbolIsTypeOnly && isJs ? { kind: ImportFixKind.ImportType, moduleSpecifier, position: Debug.assertDefined(position) } : { kind: ImportFixKind.AddNew, moduleSpecifier, importKind })); | ||
// Sort to keep the shortest paths first | ||
return sort(choicesForEachExportingModule, (a, b) => a.moduleSpecifier.length - b.moduleSpecifier.length); | ||
|
||
// Sort by presence in package.json, then shortest paths first | ||
return sort(choicesForEachExportingModule, (a, b) => { | ||
const allowsImportingA = allowsImporting(a.moduleSpecifier); | ||
const allowsImportingB = allowsImporting(b.moduleSpecifier); | ||
if (allowsImportingA && !allowsImportingB) { | ||
return -1; | ||
} | ||
if (allowsImportingB && !allowsImportingA) { | ||
return 1; | ||
} | ||
return a.moduleSpecifier.length - b.moduleSpecifier.length; | ||
}); | ||
} | ||
|
||
function getFixesForAddImport( | ||
|
@@ -380,7 +392,8 @@ namespace ts.codefix { | |
// "default" is a keyword and not a legal identifier for the import, so we don't expect it here | ||
Debug.assert(symbolName !== InternalSymbolName.Default); | ||
|
||
const fixes = arrayFrom(flatMapIterator(getExportInfos(symbolName, getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, checker, program).entries(), ([_, exportInfos]) => | ||
const exportInfos = getExportInfos(symbolName, getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, checker, program, preferences, host); | ||
const fixes = arrayFrom(flatMapIterator(exportInfos.entries(), ([_, exportInfos]) => | ||
getFixForImport(exportInfos, symbolName, symbolToken.getStart(sourceFile), program, sourceFile, host, preferences))); | ||
return { fixes, symbolName }; | ||
} | ||
|
@@ -393,14 +406,16 @@ namespace ts.codefix { | |
sourceFile: SourceFile, | ||
checker: TypeChecker, | ||
program: Program, | ||
preferences: UserPreferences, | ||
host: LanguageServiceHost | ||
): ReadonlyMap<ReadonlyArray<SymbolExportInfo>> { | ||
// For each original symbol, keep all re-exports of that symbol together so we can call `getCodeActionsForImport` on the whole group at once. | ||
// Maps symbol id to info for modules providing that symbol (original export + re-exports). | ||
const originalSymbolToExportInfos = createMultiMap<SymbolExportInfo>(); | ||
function addSymbol(moduleSymbol: Symbol, exportedSymbol: Symbol, importKind: ImportKind): void { | ||
originalSymbolToExportInfos.add(getUniqueSymbolId(exportedSymbol, checker).toString(), { moduleSymbol, importKind, exportedSymbolIsTypeOnly: isTypeOnlySymbol(exportedSymbol, checker) }); | ||
} | ||
forEachExternalModuleToImportFrom(checker, sourceFile, program.getSourceFiles(), moduleSymbol => { | ||
forEachExternalModuleToImportFrom(checker, host, preferences, program.redirectTargetsMap, sourceFile, program.getSourceFiles(), moduleSymbol => { | ||
cancellationToken.throwIfCancellationRequested(); | ||
|
||
const defaultInfo = getDefaultLikeExportInfo(moduleSymbol, checker, program.getCompilerOptions()); | ||
|
@@ -561,12 +576,44 @@ namespace ts.codefix { | |
return some(declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning)); | ||
} | ||
|
||
export function forEachExternalModuleToImportFrom(checker: TypeChecker, from: SourceFile, allSourceFiles: ReadonlyArray<SourceFile>, cb: (module: Symbol) => void) { | ||
export function forEachExternalModuleToImportFrom(checker: TypeChecker, host: LanguageServiceHost, preferences: UserPreferences, redirectTargetsMap: RedirectTargetsMap, from: SourceFile, allSourceFiles: ReadonlyArray<SourceFile>, cb: (module: Symbol) => void) { | ||
const { allowsImporting } = createLazyPackageJsonDependencyReader(from, host); | ||
const compilerOptions = host.getCompilationSettings(); | ||
const getCanonicalFileName = hostGetCanonicalFileName(host); | ||
forEachExternalModule(checker, allSourceFiles, (module, sourceFile) => { | ||
if (sourceFile === undefined || sourceFile !== from && isImportablePath(from.fileName, sourceFile.fileName)) { | ||
if (sourceFile === undefined && allowsImporting(stripQuotes(module.getName()))) { | ||
cb(module); | ||
} | ||
else if (sourceFile && sourceFile !== from && isImportablePath(from.fileName, sourceFile.fileName)) { | ||
const moduleSpecifier = getNodeModulesPackageNameFromFileName(sourceFile.fileName); | ||
if (!moduleSpecifier || allowsImporting(moduleSpecifier)) { | ||
cb(module); | ||
} | ||
} | ||
}); | ||
|
||
function getNodeModulesPackageNameFromFileName(importedFileName: string): string | undefined { | ||
const specifier = moduleSpecifiers.getModuleSpecifier( | ||
compilerOptions, | ||
from, | ||
toPath(from.fileName, /*basePath*/ undefined, getCanonicalFileName), | ||
importedFileName, | ||
host, | ||
allSourceFiles, | ||
preferences, | ||
redirectTargetsMap); | ||
|
||
// Paths here are not node_modules, so we don’t care about them; | ||
// returning anything will trigger a lookup in package.json. | ||
if (!pathIsRelative(specifier) && !isRootedDiskPath(specifier)) { | ||
const components = getPathComponents(getPackageNameFromTypesPackageName(specifier)).slice(1); | ||
// Scoped packages | ||
if (startsWith(components[0], "@")) { | ||
return `${components[0]}/${components[1]}`; | ||
} | ||
return components[0]; | ||
} | ||
} | ||
} | ||
|
||
function forEachExternalModule(checker: TypeChecker, allSourceFiles: ReadonlyArray<SourceFile>, cb: (module: Symbol, sourceFile: SourceFile | undefined) => void) { | ||
|
@@ -620,4 +667,69 @@ namespace ts.codefix { | |
// Need `|| "_"` to ensure result isn't empty. | ||
return !isStringANonContextualKeyword(res) ? res || "_" : `_${res}`; | ||
} | ||
|
||
function createLazyPackageJsonDependencyReader(fromFile: SourceFile, host: LanguageServiceHost) { | ||
const packageJsonPaths = findPackageJsons(getDirectoryPath(fromFile.fileName), host); | ||
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. I think from tsserver perspective caching packageJson should be fairely ok since we do watch failed lookup locations and any changes in there result in recomputing program. But that's not guaranteed with other hosts since they can have their own module resolution or use the default resolution cache which doesn't watch these locations. 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.
Just to make sure I’ve interpreted this correctly: you’re saying that if a user runs
Maybe we can implement a fast check to determine whether the cache is up-to-date (last modified timestamp? content hash?). Actually, that might be better than reading eagerly on recomputing the program, since the program will change more often than the package.json. |
||
const dependencyIterator = readPackageJsonDependencies(host, packageJsonPaths); | ||
let seenDeps: Map<true> | undefined; | ||
andrewbranch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let usesNodeCoreModules: boolean | undefined; | ||
return { allowsImporting }; | ||
|
||
function containsDependency(dependency: string) { | ||
if ((seenDeps || (seenDeps = createMap())).has(dependency)) { | ||
return true; | ||
} | ||
let packageName: string | void; | ||
while (packageName = dependencyIterator.next().value) { | ||
seenDeps.set(packageName, true); | ||
if (packageName === dependency) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
function allowsImporting(moduleSpecifier: string): boolean { | ||
if (!packageJsonPaths.length) { | ||
return true; | ||
} | ||
|
||
// If we’re in JavaScript, it can be difficult to tell whether the user wants to import | ||
// from Node core modules or not. We can start by seeing if the user is actually using | ||
// any node core modules, as opposed to simply having @types/node accidentally as a | ||
// dependency of a dependency. | ||
if (isSourceFileJS(fromFile) && JsTyping.nodeCoreModules.has(moduleSpecifier)) { | ||
if (usesNodeCoreModules === undefined) { | ||
usesNodeCoreModules = consumesNodeCoreModules(fromFile); | ||
} | ||
if (usesNodeCoreModules) { | ||
return true; | ||
} | ||
} | ||
|
||
return containsDependency(moduleSpecifier) | ||
|| containsDependency(getTypesPackageName(moduleSpecifier)); | ||
} | ||
} | ||
|
||
function *readPackageJsonDependencies(host: LanguageServiceHost, packageJsonPaths: string[]) { | ||
type PackageJson = Record<typeof dependencyKeys[number], Record<string, string> | undefined>; | ||
const dependencyKeys = ["dependencies", "devDependencies", "optionalDependencies"] as const; | ||
for (const fileName of packageJsonPaths) { | ||
const content = readJson(fileName, { readFile: host.readFile ? host.readFile.bind(host) : sys.readFile }) as PackageJson; | ||
for (const key of dependencyKeys) { | ||
const dependencies = content[key]; | ||
if (!dependencies) { | ||
continue; | ||
} | ||
for (const packageName in dependencies) { | ||
yield packageName; | ||
} | ||
} | ||
} | ||
} | ||
|
||
function consumesNodeCoreModules(sourceFile: SourceFile): boolean { | ||
return some(sourceFile.imports, ({ text }) => JsTyping.nodeCoreModules.has(text)); | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.