Skip to content

When directory watcher is invoked with any file from node_modules package, invalidate for file paths in that package #43974

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 4 commits into from
May 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
7 changes: 4 additions & 3 deletions src/compiler/moduleNameResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1150,7 +1150,7 @@ namespace ts {
}
const resolvedFromFile = loadModuleFromFile(extensions, candidate, onlyRecordFailures, state);
if (resolvedFromFile) {
const packageDirectory = considerPackageJson ? parseNodeModuleFromPath(resolvedFromFile) : undefined;
const packageDirectory = considerPackageJson ? parseNodeModuleFromPath(resolvedFromFile.path) : undefined;
const packageInfo = packageDirectory ? getPackageJsonInfo(packageDirectory, /*onlyRecordFailures*/ false, state) : undefined;
return withPackageId(packageInfo, resolvedFromFile);
}
Expand Down Expand Up @@ -1184,8 +1184,9 @@ namespace ts {
* For `/node_modules/@types/foo/bar/index.d.ts` this is packageDirectory: "@types/foo"
* For `/node_modules/foo/bar/index.d.ts` this is packageDirectory: "foo"
*/
function parseNodeModuleFromPath(resolved: PathAndExtension): string | undefined {
const path = normalizePath(resolved.path);
/* @internal */
export function parseNodeModuleFromPath(resolved: string): string | undefined {
const path = normalizePath(resolved);
const idx = path.lastIndexOf(nodeModulesPathPart);
if (idx === -1) {
return undefined;
Expand Down
38 changes: 22 additions & 16 deletions src/compiler/resolutionCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,9 @@ namespace ts {
const resolvedFileToResolution = createMultiMap<ResolutionWithFailedLookupLocations>();

let hasChangedAutomaticTypeDirectiveNames = false;
const failedLookupChecks: Path[] = [];
const startsWithPathChecks: Path[] = [];
const isInDirectoryChecks: Path[] = [];
let failedLookupChecks: Path[] | undefined;
let startsWithPathChecks: Set<Path> | undefined;
let isInDirectoryChecks: Path[] | undefined;

const getCurrentDirectory = memoize(() => resolutionHost.getCurrentDirectory!()); // TODO: GH#18217
const cachedDirectoryStructureHost = resolutionHost.getCachedDirectoryStructureHost();
Expand Down Expand Up @@ -248,9 +248,9 @@ namespace ts {
resolvedTypeReferenceDirectives.clear();
resolvedFileToResolution.clear();
resolutionsWithFailedLookups.length = 0;
failedLookupChecks.length = 0;
startsWithPathChecks.length = 0;
isInDirectoryChecks.length = 0;
failedLookupChecks = undefined;
startsWithPathChecks = undefined;
isInDirectoryChecks = undefined;
// perDirectoryResolvedModuleNames and perDirectoryResolvedTypeReferenceDirectives could be non empty if there was exception during program update
// (between startCachingPerDirectoryResolution and finishCachingPerDirectoryResolution)
clearPerDirectoryResolutions();
Expand Down Expand Up @@ -766,7 +766,7 @@ namespace ts {
if (isCreatingWatchedDirectory) {
// Watching directory is created
// Invalidate any resolution has failed lookup in this directory
isInDirectoryChecks.push(fileOrDirectoryPath);
(isInDirectoryChecks ||= []).push(fileOrDirectoryPath);
}
else {
// If something to do with folder/file starting with "." in node_modules folder, skip it
Expand All @@ -785,8 +785,8 @@ namespace ts {
if (isNodeModulesAtTypesDirectory(fileOrDirectoryPath) || isNodeModulesDirectory(fileOrDirectoryPath) ||
isNodeModulesAtTypesDirectory(dirOfFileOrDirectory) || isNodeModulesDirectory(dirOfFileOrDirectory)) {
// Invalidate any resolution from this directory
failedLookupChecks.push(fileOrDirectoryPath);
startsWithPathChecks.push(fileOrDirectoryPath);
(failedLookupChecks ||= []).push(fileOrDirectoryPath);
(startsWithPathChecks ||= new Set()).add(fileOrDirectoryPath);
}
else {
if (!isPathWithDefaultFailedLookupExtension(fileOrDirectoryPath) && !customFailedLookupPaths.has(fileOrDirectoryPath)) {
Expand All @@ -797,30 +797,36 @@ namespace ts {
return false;
}
// Resolution need to be invalidated if failed lookup location is same as the file or directory getting created
failedLookupChecks.push(fileOrDirectoryPath);
(failedLookupChecks ||= []).push(fileOrDirectoryPath);

// If the invalidated file is from a node_modules package, invalidate everything else
// in the package since we might not get notifications for other files in the package.
// This hardens our logic against unreliable file watchers.
const packagePath = parseNodeModuleFromPath(fileOrDirectoryPath);
if (packagePath) (startsWithPathChecks ||= new Set()).add(packagePath as Path);
}
}
resolutionHost.scheduleInvalidateResolutionsOfFailedLookupLocations();
}

function invalidateResolutionsOfFailedLookupLocations() {
if (!failedLookupChecks.length && !startsWithPathChecks.length && !isInDirectoryChecks.length) {
if (!failedLookupChecks && !startsWithPathChecks && !isInDirectoryChecks) {
return false;
}

const invalidated = invalidateResolutions(resolutionsWithFailedLookups, canInvalidateFailedLookupResolution);
failedLookupChecks.length = 0;
startsWithPathChecks.length = 0;
isInDirectoryChecks.length = 0;
failedLookupChecks = undefined;
startsWithPathChecks = undefined;
isInDirectoryChecks = undefined;
return invalidated;
}

function canInvalidateFailedLookupResolution(resolution: ResolutionWithFailedLookupLocations) {
return resolution.failedLookupLocations.some(location => {
const locationPath = resolutionHost.toPath(location);
return contains(failedLookupChecks, locationPath) ||
startsWithPathChecks.some(fileOrDirectoryPath => startsWith(locationPath, fileOrDirectoryPath)) ||
isInDirectoryChecks.some(fileOrDirectoryPath => isInDirectoryPath(fileOrDirectoryPath, locationPath));
firstDefinedIterator(startsWithPathChecks?.keys() || emptyIterator, fileOrDirectoryPath => startsWith(locationPath, fileOrDirectoryPath) ? true : undefined) ||
isInDirectoryChecks?.some(fileOrDirectoryPath => isInDirectoryPath(fileOrDirectoryPath, locationPath));
});
}

Expand Down
79 changes: 79 additions & 0 deletions src/testRunner/unittests/tscWatch/resolutionCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,5 +449,84 @@ declare namespace myapp {
},
changes: emptyArray
});

describe("works when installing something in node_modules or @types when there is no notification from fs for index file", () => {
function getNodeAtTypes() {
const nodeAtTypesIndex: File = {
path: `${projectRoot}/node_modules/@types/node/index.d.ts`,
content: `/// <reference path="base.d.ts" />`
};
const nodeAtTypesBase: File = {
path: `${projectRoot}/node_modules/@types/node/base.d.ts`,
content: `// Base definitions for all NodeJS modules that are not specific to any version of TypeScript:
/// <reference path="ts3.6/base.d.ts" />`
};
const nodeAtTypes36Base: File = {
path: `${projectRoot}/node_modules/@types/node/ts3.6/base.d.ts`,
content: `/// <reference path="../globals.d.ts" />`
};
const nodeAtTypesGlobals: File = {
path: `${projectRoot}/node_modules/@types/node/globals.d.ts`,
content: `declare var process: NodeJS.Process;
declare namespace NodeJS {
interface Process {
on(msg: string): void;
}
}`
};
return { nodeAtTypesIndex, nodeAtTypesBase, nodeAtTypes36Base, nodeAtTypesGlobals };
}
verifyTscWatch({
scenario,
subScenario: "works when installing something in node_modules or @types when there is no notification from fs for index file",
commandLineArgs: ["--w", `--extendedDiagnostics`],
sys: () => {
const file: File = {
path: `${projectRoot}/worker.ts`,
content: `process.on("uncaughtException");`
};
const tsconfig: File = {
path: `${projectRoot}/tsconfig.json`,
content: "{}"
};
const { nodeAtTypesIndex, nodeAtTypesBase, nodeAtTypes36Base, nodeAtTypesGlobals } = getNodeAtTypes();
return createWatchedSystem([file, libFile, tsconfig, nodeAtTypesIndex, nodeAtTypesBase, nodeAtTypes36Base, nodeAtTypesGlobals], { currentDirectory: projectRoot });
},
changes: [
{
caption: "npm ci step one: remove all node_modules files",
change: sys => sys.deleteFolder(`${projectRoot}/node_modules/@types`, /*recursive*/ true),
timeouts: runQueuedTimeoutCallbacks,
},
{
caption: `npm ci step two: create atTypes but something else in the @types folder`,
change: sys => sys.ensureFileOrFolder({
path: `${projectRoot}/node_modules/@types/mocha/index.d.ts`,
content: `export const foo = 10;`
}),
timeouts: runQueuedTimeoutCallbacks
},
{
caption: `npm ci step three: create atTypes node folder`,
change: sys => sys.ensureFileOrFolder({ path: `${projectRoot}/node_modules/@types/node` }),
timeouts: runQueuedTimeoutCallbacks
},
{
caption: `npm ci step four: create atTypes write all the files but dont invoke watcher for index.d.ts`,
change: sys => {
const { nodeAtTypesIndex, nodeAtTypesBase, nodeAtTypes36Base, nodeAtTypesGlobals } = getNodeAtTypes();
sys.ensureFileOrFolder(nodeAtTypesBase);
sys.ensureFileOrFolder(nodeAtTypesIndex, /*ignoreWatchInvokedWithTriggerAsFileCreate*/ true);
sys.ensureFileOrFolder(nodeAtTypes36Base, /*ignoreWatchInvokedWithTriggerAsFileCreate*/ true);
sys.ensureFileOrFolder(nodeAtTypesGlobals, /*ignoreWatchInvokedWithTriggerAsFileCreate*/ true);
},
timeouts: sys => {
sys.runQueuedTimeoutCallbacks(); // update failed lookups
sys.runQueuedTimeoutCallbacks(); // actual program update
},
},
]
});
});
});
}
Loading