Skip to content

Include actual generated module specifiers in module specifier cache #44176

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

Merged
merged 14 commits into from
Jun 10, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
49 changes: 38 additions & 11 deletions src/compiler/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,19 @@ namespace ts.moduleSpecifiers {
if (!moduleSourceFile) {
return [];
}
const modulePaths = getAllModulePaths(importingSourceFile.path, moduleSourceFile.originalFileName, host);
const preferences = getPreferences(userPreferences, compilerOptions, importingSourceFile);

const cache = host.getModuleSpecifierCache?.();
const cached = cache?.get(importingSourceFile.path, moduleSourceFile.path);
let modulePaths;
if (typeof cached === "object") {
if (cached.moduleSpecifiers) return cached.moduleSpecifiers;
modulePaths = cached.modulePaths;
}
else {
modulePaths = getAllModulePathsWorker(importingSourceFile.path, moduleSourceFile.originalFileName, host);
}

const preferences = getPreferences(userPreferences, compilerOptions, importingSourceFile);
Copy link
Member

Choose a reason for hiding this comment

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

Should user preferences be part of the key for the cache as it does affect the module specifiers (esp what if compileOnSave with declarations enabled which uses module specifiers(? but not sure if same path applies) and auto import are inter mixed?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Only importModuleSpecifierEnding and importModuleSpecifierPreference affect the answers and I’m currently clearing it in editorServices.ts when they change... but it might be cleaner to make it part of the key. I’ll look and see how disruptive that is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Latest commit makes it part of the cache key instead of clearing when the host config changes. It has to be threaded through a lot more places, but it’s definitely more obvious that it’s a dependency, and will play nicer with other hosts/callers.

const existingSpecifier = forEach(modulePaths, modulePath => forEach(
host.getFileIncludeReasons().get(toPath(modulePath.path, host.getCurrentDirectory(), info.getCanonicalFileName)),
reason => {
Expand All @@ -120,7 +130,11 @@ namespace ts.moduleSpecifiers {
undefined;
}
));
if (existingSpecifier) return [existingSpecifier];
if (existingSpecifier) {
const moduleSpecifiers = [existingSpecifier];
cache?.set(importingSourceFile.path, moduleSourceFile.path, { modulePaths, moduleSpecifiers });
return moduleSpecifiers;
}

const importedFileIsInNodeModules = some(modulePaths, p => p.isInNodeModules);

Expand All @@ -138,6 +152,7 @@ namespace ts.moduleSpecifiers {
if (specifier && modulePath.isRedirect) {
// If we got a specifier for a redirect, it was a bare package specifier (e.g. "@foo/bar",
// not "@foo/bar/path/to/file"). No other specifier will be this good, so stop looking.
cache?.set(importingSourceFile.path, moduleSourceFile.path, { modulePaths, moduleSpecifiers: nodeModulesSpecifiers! });
return nodeModulesSpecifiers!;
}

Expand All @@ -161,9 +176,11 @@ namespace ts.moduleSpecifiers {
}
}

return pathsSpecifiers?.length ? pathsSpecifiers :
const moduleSpecifiers = pathsSpecifiers?.length ? pathsSpecifiers :
nodeModulesSpecifiers?.length ? nodeModulesSpecifiers :
Debug.checkDefined(relativeSpecifiers);
cache?.set(importingSourceFile.path, moduleSourceFile.path, { modulePaths, moduleSpecifiers });
return moduleSpecifiers;
}

interface Info {
Expand Down Expand Up @@ -332,13 +349,26 @@ namespace ts.moduleSpecifiers {
* Looks for existing imports that use symlinks to this module.
* Symlinks will be returned first so they are preferred over the real path.
*/
function getAllModulePaths(importingFileName: Path, importedFileName: string, host: ModuleSpecifierResolutionHost): readonly ModulePath[] {
function getAllModulePaths(
importingFilePath: Path,
importedFileName: string,
host: ModuleSpecifierResolutionHost,
importedFilePath = toPath(importedFileName, host.getCurrentDirectory(), hostGetCanonicalFileName(host))
) {
const cache = host.getModuleSpecifierCache?.();
const getCanonicalFileName = hostGetCanonicalFileName(host);
if (cache) {
const cached = cache.get(importingFileName, toPath(importedFileName, host.getCurrentDirectory(), getCanonicalFileName));
if (typeof cached === "object") return cached;
const cached = cache.get(importingFilePath, importedFilePath);
if (typeof cached === "object") return cached.modulePaths;
}
const modulePaths = getAllModulePathsWorker(importingFilePath, importedFileName, host);
if (cache) {
cache.setModulePaths(importingFilePath, importedFilePath, modulePaths);
}
return modulePaths;
}

function getAllModulePathsWorker(importingFileName: Path, importedFileName: string, host: ModuleSpecifierResolutionHost): readonly ModulePath[] {
const getCanonicalFileName = hostGetCanonicalFileName(host);
const allFileNames = new Map<string, { path: string, isRedirect: boolean, isInNodeModules: boolean }>();
let importedFileFromNodeModules = false;
forEachFileNameOfModule(
Expand Down Expand Up @@ -384,9 +414,6 @@ namespace ts.moduleSpecifiers {
sortedPaths.push(...remainingPaths);
}

if (cache) {
cache.set(importingFileName, toPath(importedFileName, host.getCurrentDirectory(), getCanonicalFileName), sortedPaths);
}
return sortedPaths;
}

Expand Down
6 changes: 4 additions & 2 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8108,8 +8108,10 @@ namespace ts {

/* @internal */
export interface ModuleSpecifierCache {
get(fromFileName: Path, toFileName: Path): boolean | readonly ModulePath[] | undefined;
set(fromFileName: Path, toFileName: Path, moduleSpecifiers: boolean | readonly ModulePath[]): void;
getModulePaths(fromFileName: Path, toFileName: Path): boolean | readonly ModulePath[] | undefined;
setModulePaths(fromFileName: Path, toFileName: Path, modulePaths: boolean | readonly ModulePath[]): void;
get(fromFileName: Path, toFileName: Path): { modulePaths: readonly ModulePath[], moduleSpecifiers?: readonly string[] } | boolean | undefined;
set(fromFileName: Path, toFileName: Path, cached: { modulePaths: readonly ModulePath[], moduleSpecifiers?: readonly string[] }): void;
clear(): void;
count(): number;
}
Expand Down
13 changes: 12 additions & 1 deletion src/server/editorServices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2945,7 +2945,13 @@ namespace ts.server {
this.logger.info("Format host information updated");
}
if (args.preferences) {
const { lazyConfiguredProjectsFromExternalProject, includePackageJsonAutoImports } = this.hostConfiguration.preferences;
const {
lazyConfiguredProjectsFromExternalProject,
includePackageJsonAutoImports,
importModuleSpecifierEnding,
importModuleSpecifierPreference,
} = this.hostConfiguration.preferences;

this.hostConfiguration.preferences = { ...this.hostConfiguration.preferences, ...args.preferences };
if (lazyConfiguredProjectsFromExternalProject && !this.hostConfiguration.preferences.lazyConfiguredProjectsFromExternalProject) {
// Load configured projects for external projects that are pending reload
Expand All @@ -2960,6 +2966,11 @@ namespace ts.server {
if (includePackageJsonAutoImports !== args.preferences.includePackageJsonAutoImports) {
this.invalidateProjectPackageJson(/*packageJsonPath*/ undefined);
}
if (importModuleSpecifierEnding !== args.preferences.importModuleSpecifierEnding || importModuleSpecifierPreference !== args.preferences.importModuleSpecifierPreference) {
this.configuredProjects.forEach(p => p.getModuleSpecifierCache().clear());
this.inferredProjects.forEach(p => p.getModuleSpecifierCache().clear());
this.externalProjects.forEach(p => p.getModuleSpecifierCache().clear());
}
}
if (args.extraFileExtensions) {
this.hostConfiguration.extraFileExtensions = args.extraFileExtensions;
Expand Down
14 changes: 13 additions & 1 deletion src/server/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ namespace ts.server {
/*@internal*/
private changedFilesForExportMapCache: Set<Path> | undefined;
/*@internal*/
private moduleSpecifierCache = createModuleSpecifierCache();
private moduleSpecifierCache = createModuleSpecifierCache(this);
/*@internal*/
private symlinks: SymlinkCache | undefined;
/*@internal*/
Expand Down Expand Up @@ -1389,6 +1389,7 @@ namespace ts.server {
this.cachedUnresolvedImportsPerFile.clear();
this.lastCachedUnresolvedImportsList = undefined;
this.resolutionCache.clear();
this.moduleSpecifierCache.clear();
}
this.markAsDirty();
}
Expand Down Expand Up @@ -1730,6 +1731,17 @@ namespace ts.server {
this.projectService.openFiles,
(_, fileName) => this.projectService.tryGetDefaultProjectForFile(toNormalizedPath(fileName)) === this);
}

/*@internal*/
watchNodeModulesDirectory(directoryPath: string, cb: DirectoryWatcherCallback) {
return this.projectService.watchFactory.watchDirectory(
directoryPath,
cb,
WatchDirectoryFlags.Recursive,
this.projectService.getWatchOptions(this),
WatchType.NodeModulesForClosedScriptInfo
);
}
}

function getUnresolvedImports(program: Program, cachedUnresolvedImportsPerFile: ESMap<Path, readonly string[]>): SortedReadonlyArray<string> {
Expand Down
6 changes: 4 additions & 2 deletions src/server/watchType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ namespace ts {
MissingSourceMapFile: "Missing source map file",
NoopConfigFileForInferredRoot: "Noop Config file for the inferred project root",
MissingGeneratedFile: "Missing generated file",
PackageJsonFile: "package.json file for import suggestions"
PackageJsonFile: "package.json file for import suggestions",
NodeModulesForModuleSpecifierCache: "node_modules for module specifier cache invalidation",
}
WatchType.ClosedScriptInfo = "Closed Script info";
WatchType.ConfigFileForInferredRoot = "Config file for the inferred project root";
Expand All @@ -17,4 +18,5 @@ namespace ts {
WatchType.NoopConfigFileForInferredRoot = "Noop Config file for the inferred project root";
WatchType.MissingGeneratedFile = "Missing generated file";
WatchType.PackageJsonFile = "package.json file for import suggestions";
}
WatchType.NodeModulesForModuleSpecifierCache = "node_modules for module specifier cache invalidation";
}
54 changes: 46 additions & 8 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3273,23 +3273,61 @@ namespace ts {
}
}

export function createModuleSpecifierCache(): ModuleSpecifierCache {
let cache: ESMap<Path, boolean | readonly ModulePath[]> | undefined;
export interface ModuleSpecifierResolutionCacheHost {
watchNodeModulesDirectory(directoryPath: string, cb: DirectoryWatcherCallback): FileWatcher;
}

export function createModuleSpecifierCache(host: ModuleSpecifierResolutionCacheHost): ModuleSpecifierCache {
let containedNodeModulesWatchers: ESMap<string, FileWatcher> | undefined;
let cache: ESMap<Path, boolean | { modulePaths: readonly ModulePath[], moduleSpecifiers?: readonly string[] }> | undefined;
let importingFileName: Path | undefined;
const wrapped: ModuleSpecifierCache = {
get(fromFileName, toFileName) {
if (!cache || fromFileName !== importingFileName) return undefined;
return cache.get(toFileName);
},
set(fromFileName, toFileName, moduleSpecifiers) {
set(fromFileName, toFileName, cached) {
if (cache && fromFileName !== importingFileName) {
this.clear();
}
importingFileName = fromFileName;
(cache ||= new Map()).set(toFileName, cached);

// If any module specifiers were generated based off paths in node_modules,
// a package.json file in that package was read and is an input to the cached.
// Instead of watching each individual package.json file, set up a wildcard
// directory watcher for any node_modules referenced and clear the cache when
// it sees any changes.
if (cached.moduleSpecifiers) {
for (const p of cached.modulePaths) {
if (p.isInNodeModules) {
const nodeModulesPath = p.path.substring(0, p.path.indexOf(nodeModulesPathPart) + nodeModulesPathPart.length);
if (!containedNodeModulesWatchers?.has(nodeModulesPath)) {
(containedNodeModulesWatchers ||= new Map()).set(
nodeModulesPath,
host.watchNodeModulesDirectory(nodeModulesPath, this.clear),
);
}
}
}
}
},
getModulePaths(fromFileName, toFileName) {
if (!cache || fromFileName !== importingFileName) return undefined;
const cached = cache.get(toFileName);
return typeof cached === "object" ? cached.modulePaths : cached;
},
setModulePaths(fromFileName, toFileName, modulePaths) {
if (cache && fromFileName !== importingFileName) {
cache.clear();
this.clear();
}
importingFileName = fromFileName;
(cache ||= new Map()).set(toFileName, moduleSpecifiers);
(cache ||= new Map()).set(toFileName, typeof modulePaths === "boolean" ? modulePaths : { modulePaths });
},
clear() {
cache = undefined;
containedNodeModulesWatchers?.forEach(watcher => watcher.close());
cache?.clear();
containedNodeModulesWatchers?.clear();
importingFileName = undefined;
},
count() {
Expand All @@ -3311,7 +3349,7 @@ namespace ts {
moduleSpecifierCache: ModuleSpecifierCache | undefined,
): boolean {
if (from === to) return false;
const cachedResult = moduleSpecifierCache?.get(from.path, to.path);
const cachedResult = moduleSpecifierCache?.getModulePaths(from.path, to.path);
if (cachedResult !== undefined) {
return !!cachedResult;
}
Expand All @@ -3334,7 +3372,7 @@ namespace ts {

if (packageJsonFilter) {
const isImportable = hasImportablePath && packageJsonFilter.allowsImportingSourceFile(to, moduleSpecifierResolutionHost);
moduleSpecifierCache?.set(from.path, to.path, isImportable);
moduleSpecifierCache?.setModulePaths(from.path, to.path, isImportable);
return isImportable;
}

Expand Down
Loading