Skip to content
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

Proposal: If there’s a package.json, only auto-import things in it, more or less #31893

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
40321ad
Move package.json related utils to utilities
andrewbranch Jun 10, 2019
b43f088
Add failing test
andrewbranch Jun 10, 2019
7af4d37
Make first test pass
andrewbranch Jun 11, 2019
ab0339c
Don’t filter when there’s no package.json, fix scoped package imports
andrewbranch Jun 11, 2019
a1a9b55
Use type acquisition as a heuristic for whether a JS project is using…
andrewbranch Jun 12, 2019
afa8461
Make same fix in getCompletionDetails
andrewbranch Jun 12, 2019
e625471
Fix re-exporting
andrewbranch Jun 13, 2019
921a1b7
Change JS node core module heuristic to same-file utilization
andrewbranch Jun 13, 2019
97c00b6
Remove unused method
andrewbranch Jun 13, 2019
4945c87
Remove other unused method
andrewbranch Jun 13, 2019
2d7eaf9
Remove unused triple-slash ref
andrewbranch Jun 13, 2019
306afe6
Update comment
andrewbranch Jun 13, 2019
488fa8f
Refactor findAlias to forEachAlias to reduce iterations
andrewbranch Jun 13, 2019
98436c9
Really fix re-exporting
andrewbranch Jun 13, 2019
cb2eef9
Use getModuleSpecifier instead of custom hack
andrewbranch Jun 14, 2019
7639059
Fix offering auto imports to paths within node modules
andrewbranch Jun 14, 2019
d6767a8
Rename things and make comments better
andrewbranch Jun 26, 2019
7268bca
Add another reexport test
andrewbranch Jun 26, 2019
3962d4a
Inline `symbolHasBeenSeen`
andrewbranch Jul 11, 2019
b2ce461
Simplify forEachAlias to findAlias
andrewbranch Jul 11, 2019
f276466
Add note that symbols is mutated
andrewbranch Jul 11, 2019
ecd15e5
Symbol order doesn’t matter here
andrewbranch Jul 11, 2019
ac0f70a
Style nits
andrewbranch Jul 11, 2019
fbb05d5
Add test with nested package.jsons
andrewbranch Jul 11, 2019
de8ef32
Fix and add tests for export * re-exports
andrewbranch Jul 12, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7468,7 +7468,7 @@ namespace ts {
export function getDirectoryPath(path: Path): Path;
/**
* Returns the path except for its basename. Semantics align with NodeJS's `path.dirname`
* except that we support URL's as well.
* except that we support URLs as well.
*
* ```ts
* getDirectoryPath("/path/to/file.ext") === "/path/to"
Expand Down
2 changes: 1 addition & 1 deletion src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ namespace FourSlash {
const name = typeof include === "string" ? include : include.name;
const found = nameToEntries.get(name);
if (!found) throw this.raiseError(`No completion ${name} found`);
assert(found.length === 1); // Must use 'exact' for multiple completions with same name
assert(found.length === 1, `Must use 'exact' for multiple completions with same name: '${name}'`);
this.verifyCompletionEntry(ts.first(found), include);
}
}
Expand Down
124 changes: 118 additions & 6 deletions src/services/codefixes/importFixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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 };
}
Expand All @@ -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());
Expand Down Expand Up @@ -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) {
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
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) {
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

we do watch failed lookup locations and any changes in there result in recomputing program

Just to make sure I’ve interpreted this correctly: you’re saying that if a user runs npm install new-package which changes the package.json, synchronizeHostData will be called so I could update the cache there?

But that's not guaranteed with other hosts

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));
}
}
Loading