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.

Sign up for GitHub

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

Include actual generated module specifiers in module specifier cache #44176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0e722f0
Add cache invalidation for node_modules, config, and preferences changes
andrewbranch May 19, 2021
8063299
Share watches with closed script info
andrewbranch Jun 4, 2021
8f2c5d1
Update API baseline
andrewbranch Jun 4, 2021
b3bb946
Small adjustments for fewer object allocations
andrewbranch Jun 7, 2021
1179003
Document overloaded return value
andrewbranch Jun 7, 2021
5582f48
Merge branch 'master' into perf/module-specifier-cache
andrewbranch Jun 7, 2021
fdd76be
Merge branch 'main' into perf/module-specifier-cache
andrewbranch Jun 7, 2021
7ee4dc7
Update baselines
andrewbranch Jun 7, 2021
1eaf854
Store isAutoImportable separately from modulePaths
andrewbranch Jun 8, 2021
24c07ac
Add back missing return
andrewbranch Jun 8, 2021
90a6c19
Return wrapped watcher instead of underlying one
andrewbranch Jun 8, 2021
c7b6f1f
Make user preferences part of the cache key instead of implicitly cle…
andrewbranch Jun 8, 2021
d4f2633
Merge branch 'main' into perf/module-specifier-cache
andrewbranch Jun 8, 2021
4604dde
Fix missed merge conflict
andrewbranch Jun 8, 2021
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
61 changes: 44 additions & 17 deletions src/compiler/moduleSpecifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ namespace ts.moduleSpecifiers {
host: ModuleSpecifierResolutionHost,
oldImportSpecifier: string,
): string | undefined {
const res = getModuleSpecifierWorker(compilerOptions, importingSourceFileName, toFileName, host, getPreferencesForUpdate(compilerOptions, oldImportSpecifier));
const res = getModuleSpecifierWorker(compilerOptions, importingSourceFileName, toFileName, host, getPreferencesForUpdate(compilerOptions, oldImportSpecifier), {});
if (res === oldImportSpecifier) return undefined;
return res;
}
Expand All @@ -59,19 +59,19 @@ namespace ts.moduleSpecifiers {
importingSourceFileName: Path,
toFileName: string,
host: ModuleSpecifierResolutionHost,
preferences: UserPreferences = {},
): string {
return getModuleSpecifierWorker(compilerOptions, importingSourceFileName, toFileName, host, getPreferences(preferences, compilerOptions, importingSourceFile));
return getModuleSpecifierWorker(compilerOptions, importingSourceFileName, toFileName, host, getPreferences({}, compilerOptions, importingSourceFile), {});
}

export function getNodeModulesPackageName(
compilerOptions: CompilerOptions,
importingSourceFileName: Path,
nodeModulesFileName: string,
host: ModuleSpecifierResolutionHost,
preferences: UserPreferences,
): string | undefined {
const info = getInfo(importingSourceFileName, host);
const modulePaths = getAllModulePaths(importingSourceFileName, nodeModulesFileName, host);
const modulePaths = getAllModulePaths(importingSourceFileName, nodeModulesFileName, host, preferences);
return firstDefined(modulePaths,
modulePath => tryGetModuleNameAsNodeModule(modulePath, info, host, compilerOptions, /*packageNameOnly*/ true));
}
Expand All @@ -81,10 +81,11 @@ namespace ts.moduleSpecifiers {
importingSourceFileName: Path,
toFileName: string,
host: ModuleSpecifierResolutionHost,
preferences: Preferences
preferences: Preferences,
userPreferences: UserPreferences,
): string {
const info = getInfo(importingSourceFileName, host);
const modulePaths = getAllModulePaths(importingSourceFileName, toFileName, host);
const modulePaths = getAllModulePaths(importingSourceFileName, toFileName, host, userPreferences);
return firstDefined(modulePaths, modulePath => tryGetModuleNameAsNodeModule(modulePath, info, host, compilerOptions)) ||
getLocalModuleSpecifier(toFileName, info, compilerOptions, host, preferences);
}
Expand All @@ -106,9 +107,17 @@ 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, userPreferences);
let modulePaths;
if (cached) {
if (cached.moduleSpecifiers) return cached.moduleSpecifiers;
modulePaths = cached.modulePaths;
}

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 +129,11 @@ namespace ts.moduleSpecifiers {
undefined;
}
));
if (existingSpecifier) return [existingSpecifier];
if (existingSpecifier) {
const moduleSpecifiers = [existingSpecifier];
cache?.set(importingSourceFile.path, moduleSourceFile.path, userPreferences, modulePaths, moduleSpecifiers);
return moduleSpecifiers;
}

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

Expand All @@ -138,6 +151,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, userPreferences, modulePaths, nodeModulesSpecifiers!);
return nodeModulesSpecifiers!;
}

Expand All @@ -161,9 +175,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, userPreferences, modulePaths, moduleSpecifiers);
return moduleSpecifiers;
}

interface Info {
Expand Down Expand Up @@ -329,13 +345,27 @@ 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,
preferences: UserPreferences,
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, preferences);
if (cached?.modulePaths) return cached.modulePaths;
}
const modulePaths = getAllModulePathsWorker(importingFilePath, importedFileName, host);
if (cache) {
cache.setModulePaths(importingFilePath, importedFilePath, preferences, 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 @@ -381,9 +411,6 @@ namespace ts.moduleSpecifiers {
sortedPaths.push(...remainingPaths);
}

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

Expand Down
1 change: 0 additions & 1 deletion src/compiler/transformers/declarations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,6 @@ namespace ts {
toPath(outputFilePath, host.getCurrentDirectory(), host.getCanonicalFileName),
toPath(declFileName, host.getCurrentDirectory(), host.getCanonicalFileName),
host,
/*preferences*/ undefined,
);
if (!pathIsRelative(specifier)) {
// If some compiler option/symlink/whatever allows access to the file containing the ambient module declaration
Expand Down
13 changes: 11 additions & 2 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8154,10 +8154,19 @@ namespace ts {
isRedirect: boolean;
}

/*@internal*/
export interface ResolvedModuleSpecifierInfo {
modulePaths: readonly ModulePath[] | undefined;
moduleSpecifiers: readonly string[] | undefined;
isAutoImportable: boolean | undefined;
}

/* @internal */
export interface ModuleSpecifierCache {
get(fromFileName: Path, toFileName: Path): boolean | readonly ModulePath[] | undefined;
set(fromFileName: Path, toFileName: Path, moduleSpecifiers: boolean | readonly ModulePath[]): void;
get(fromFileName: Path, toFileName: Path, preferences: UserPreferences): Readonly<ResolvedModuleSpecifierInfo> | undefined;
set(fromFileName: Path, toFileName: Path, preferences: UserPreferences, modulePaths: readonly ModulePath[], moduleSpecifiers: readonly string[]): void;
setIsAutoImportable(fromFileName: Path, toFileName: Path, preferences: UserPreferences, isAutoImportable: boolean): void;
setModulePaths(fromFileName: Path, toFileName: Path, preferences: UserPreferences, modulePaths: readonly ModulePath[]): void;
clear(): void;
count(): number;
}
Expand Down
107 changes: 71 additions & 36 deletions src/server/editorServices.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,8 +589,11 @@ namespace ts.server {
);
}

interface ScriptInfoInNodeModulesWatcher extends FileWatcher {
refCount: number;
interface NodeModulesWatcher extends FileWatcher {
/** How many watchers of this directory were for closed ScriptInfo */
refreshScriptInfoRefCount: number;
/** List of project names whose module specifier cache should be cleared when package.jsons change */
affectedModuleSpecifierCacheProjects?: Set<string>;
}

function getDetailWatchInfo(watchType: WatchType, project: Project | NormalizedPath | undefined) {
Expand Down Expand Up @@ -676,7 +679,7 @@ namespace ts.server {
*/
/*@internal*/
readonly filenameToScriptInfo = new Map<string, ScriptInfo>();
private readonly scriptInfoInNodeModulesWatchers = new Map<string, ScriptInfoInNodeModulesWatcher>();
private readonly nodeModulesWatchers = new Map<string, NodeModulesWatcher>();
/**
* Contains all the deleted script info's version information so that
* it does not reset when creating script info again
Expand Down Expand Up @@ -2626,59 +2629,87 @@ namespace ts.server {
}
}

private watchClosedScriptInfoInNodeModules(dir: Path): ScriptInfoInNodeModulesWatcher {
// Watch only directory
const existing = this.scriptInfoInNodeModulesWatchers.get(dir);
if (existing) {
existing.refCount++;
return existing;
}

const watchDir = dir + "/node_modules" as Path;
private createNodeModulesWatcher(dir: Path) {
const watcher = this.watchFactory.watchDirectory(
watchDir,
dir,
fileOrDirectory => {
const fileOrDirectoryPath = removeIgnoredPath(this.toPath(fileOrDirectory));
if (!fileOrDirectoryPath) return;

// Has extension
Debug.assert(result.refCount > 0);
if (watchDir === fileOrDirectoryPath) {
this.refreshScriptInfosInDirectory(watchDir);
// Clear module specifier cache for any projects whose cache was affected by
// dependency package.jsons in this node_modules directory
const basename = getBaseFileName(fileOrDirectoryPath);
if (result.affectedModuleSpecifierCacheProjects?.size && (
basename === "package.json" || basename === "node_modules"
)) {
result.affectedModuleSpecifierCacheProjects.forEach(projectName => {
this.findProject(projectName)?.getModuleSpecifierCache()?.clear();
});
}
else {
const info = this.getScriptInfoForPath(fileOrDirectoryPath);
if (info) {
if (isScriptInfoWatchedFromNodeModules(info)) {
this.refreshScriptInfo(info);
}

// Refresh closed script info after an npm install
if (result.refreshScriptInfoRefCount) {
if (dir === fileOrDirectoryPath) {
this.refreshScriptInfosInDirectory(dir);
}
// Folder
else if (!hasExtension(fileOrDirectoryPath)) {
this.refreshScriptInfosInDirectory(fileOrDirectoryPath);
else {
const info = this.getScriptInfoForPath(fileOrDirectoryPath);
if (info) {
if (isScriptInfoWatchedFromNodeModules(info)) {
this.refreshScriptInfo(info);
}
}
// Folder
else if (!hasExtension(fileOrDirectoryPath)) {
this.refreshScriptInfosInDirectory(fileOrDirectoryPath);
}
}
}
},
WatchDirectoryFlags.Recursive,
this.hostConfiguration.watchOptions,
WatchType.NodeModulesForClosedScriptInfo
WatchType.NodeModules
);
const result: ScriptInfoInNodeModulesWatcher = {
const result: NodeModulesWatcher = {
refreshScriptInfoRefCount: 0,
affectedModuleSpecifierCacheProjects: undefined,
close: () => {
if (result.refCount === 1) {
if (!result.refreshScriptInfoRefCount && !result.affectedModuleSpecifierCacheProjects?.size) {
watcher.close();
this.scriptInfoInNodeModulesWatchers.delete(dir);
}
else {
result.refCount--;
this.nodeModulesWatchers.delete(dir);
}
},
refCount: 1
};
this.scriptInfoInNodeModulesWatchers.set(dir, result);
this.nodeModulesWatchers.set(dir, result);
return result;
}

/*@internal*/
watchPackageJsonsInNodeModules(dir: Path, project: Project): FileWatcher {
const watcher = this.nodeModulesWatchers.get(dir) || this.createNodeModulesWatcher(dir);
(watcher.affectedModuleSpecifierCacheProjects ||= new Set()).add(project.getProjectName());

return {
close: () => {
watcher.affectedModuleSpecifierCacheProjects?.delete(project.getProjectName());
watcher.close();
},
};
}

private watchClosedScriptInfoInNodeModules(dir: Path): FileWatcher {
const watchDir = dir + "/node_modules" as Path;
const watcher = this.nodeModulesWatchers.get(watchDir) || this.createNodeModulesWatcher(watchDir);
watcher.refreshScriptInfoRefCount++;

return {
close: () => {
watcher.refreshScriptInfoRefCount--;
watcher.close();
},
};
}

private getModifiedTime(info: ScriptInfo) {
return (this.host.getModifiedTime!(info.path) || missingFileModifiedTime).getTime();
}
Expand Down Expand Up @@ -2954,7 +2985,11 @@ namespace ts.server {
this.logger.info("Format host information updated");
}
if (args.preferences) {
const { lazyConfiguredProjectsFromExternalProject, includePackageJsonAutoImports } = this.hostConfiguration.preferences;
const {
lazyConfiguredProjectsFromExternalProject,
includePackageJsonAutoImports,
} = 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 Down
9 changes: 8 additions & 1 deletion src/server/project.ts
View file
Open in desktop
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 @@ -790,6 +790,7 @@ namespace ts.server {
this.resolutionCache.clear();
this.resolutionCache = undefined!;
this.cachedUnresolvedImportsPerFile = undefined!;
this.moduleSpecifierCache = undefined!;
this.directoryStructureHost = undefined!;
this.projectErrors = undefined;

Expand Down Expand Up @@ -1394,6 +1395,7 @@ namespace ts.server {
this.cachedUnresolvedImportsPerFile.clear();
this.lastCachedUnresolvedImportsList = undefined;
this.resolutionCache.clear();
this.moduleSpecifierCache.clear();
}
this.markAsDirty();
}
Expand Down Expand Up @@ -1735,6 +1737,11 @@ namespace ts.server {
this.projectService.openFiles,
(_, fileName) => this.projectService.tryGetDefaultProjectForFile(toNormalizedPath(fileName)) === this);
}

/*@internal*/
watchNodeModulesForPackageJsonChanges(directoryPath: string) {
return this.projectService.watchPackageJsonsInNodeModules(this.toPath(directoryPath), this);
}
}

function getUnresolvedImports(program: Program, cachedUnresolvedImportsPerFile: ESMap<Path, readonly string[]>): SortedReadonlyArray<string> {
Expand Down
Loading