-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Improvements to tsc --watch #17669
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
Improvements to tsc --watch #17669
Changes from 1 commit
802e283
499fabc
273569f
94a589b
ef5935b
e068475
6237b22
9b18f7b
85b9254
69e5abd
c814d8e
2dd6aed
89c61e7
bb91b32
46e3d1c
031a637
0d5e6c9
2762232
65a6ee0
d55150c
8dc6248
7474ba7
6385f7e
d0a23bb
59d07dc
9895082
f1b1b12
136b091
6bf9133
a99c04e
b66b752
e639ceb
8deef58
60e2e68
3908325
7173da2
e500be2
6227a36
55931c4
e65df12
e711238
3b85f3f
ea95f3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…ed module resolution
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -768,6 +768,8 @@ namespace ts { | |
return !host.directoryExists || host.directoryExists(directoryName); | ||
} | ||
|
||
export type HasInvalidatedResolution = (sourceFile: Path) => boolean; | ||
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 don't think this type belongs in this file if it isn't used here. |
||
|
||
/** | ||
* @param {boolean} onlyRecordFailures - if true then function won't try to actually load files but instead record all attempts as failures. This flag is necessary | ||
* in cases when we know upfront that all load attempts will fail (because containing folder does not exists) however we still need to record all failed lookup locations. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -394,7 +394,7 @@ namespace ts { | |
} | ||
|
||
export function isProgramUptoDate(program: Program, rootFileNames: string[], newOptions: CompilerOptions, | ||
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 don't think this function belongs here since it isn't used in this file. 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. This belongs here because we want this as an api |
||
getSourceVersion: (path: Path) => string, fileExists: (fileName: string) => boolean): boolean { | ||
getSourceVersion: (path: Path) => string, fileExists: (fileName: string) => boolean, hasInvalidatedResolution: HasInvalidatedResolution): boolean { | ||
// If we haven't create a program yet, then it is not up-to-date | ||
if (!program) { | ||
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. Nit: type should be |
||
return false; | ||
|
@@ -432,10 +432,9 @@ namespace ts { | |
return true; | ||
|
||
function sourceFileUpToDate(sourceFile: SourceFile): boolean { | ||
if (!sourceFile) { | ||
return false; | ||
} | ||
return sourceFile.version === getSourceVersion(sourceFile.path); | ||
return sourceFile && | ||
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. Nit: input type should be |
||
sourceFile.version === getSourceVersion(sourceFile.path) && | ||
!hasInvalidatedResolution(sourceFile.path); | ||
} | ||
} | ||
|
||
|
@@ -565,6 +564,7 @@ namespace ts { | |
|
||
let moduleResolutionCache: ModuleResolutionCache; | ||
let resolveModuleNamesWorker: (moduleNames: string[], containingFile: string) => ResolvedModuleFull[]; | ||
const hasInvalidatedResolution = host.hasInvalidatedResolution || noop; | ||
if (host.resolveModuleNames) { | ||
resolveModuleNamesWorker = (moduleNames, containingFile) => host.resolveModuleNames(checkAllDefined(moduleNames), containingFile).map(resolved => { | ||
// An older host may have omitted extension, in which case we should infer it from the file extension of resolvedFileName. | ||
|
@@ -803,7 +803,7 @@ namespace ts { | |
trace(host, Diagnostics.Module_0_was_resolved_as_locally_declared_ambient_module_in_file_1, moduleName, containingFile); | ||
} | ||
} | ||
else { | ||
else if (!hasInvalidatedResolution(oldProgramState.file.path)) { | ||
resolvesToAmbientModuleInNonModifiedFile = moduleNameResolvesToAmbientModuleInNonModifiedFile(moduleName, oldProgramState); | ||
} | ||
|
||
|
@@ -962,6 +962,13 @@ namespace ts { | |
// tentatively approve the file | ||
modifiedSourceFiles.push({ oldFile: oldSourceFile, newFile: newSourceFile }); | ||
} | ||
else if (hasInvalidatedResolution(oldSourceFile.path)) { | ||
// 'module/types' references could have changed | ||
oldProgram.structureIsReused = StructureIsReused.SafeModules; | ||
|
||
// add file to the modified list so that we will resolve it later | ||
modifiedSourceFiles.push({ oldFile: oldSourceFile, newFile: newSourceFile }); | ||
} | ||
|
||
// if file has passed all checks it should be safe to reuse it | ||
newSourceFiles.push(newSourceFile); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,18 @@ | |
namespace ts { | ||
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. How does this relate to the caching already in |
||
export interface ResolutionCache { | ||
setModuleResolutionHost(host: ModuleResolutionHost): void; | ||
|
||
startRecordingFilesWithChangedResolutions(): void; | ||
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. Instead of two separate functions, how about 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. Not having to create lambdas seem better option. |
||
finishRecordingFilesWithChangedResolutions(): Path[]; | ||
|
||
resolveModuleNames(moduleNames: string[], containingFile: string, logChanges: boolean): ResolvedModuleFull[]; | ||
resolveTypeReferenceDirectives(typeDirectiveNames: string[], containingFile: string): ResolvedTypeReferenceDirective[]; | ||
|
||
invalidateResolutionOfDeletedFile(filePath: Path): void; | ||
invalidateResolutionOfChangedFailedLookupLocation(failedLookupLocation: string): void; | ||
|
||
createHasInvalidatedResolution(): HasInvalidatedResolution; | ||
|
||
clear(): void; | ||
} | ||
|
||
|
@@ -40,6 +46,7 @@ namespace ts { | |
|
||
let host: ModuleResolutionHost; | ||
let filesWithChangedSetOfUnresolvedImports: Path[]; | ||
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.
|
||
let filesWithInvalidatedResolutions: Map<true>; | ||
|
||
const resolvedModuleNames = createMap<Map<ResolvedModuleWithFailedLookupLocations>>(); | ||
const resolvedTypeReferenceDirectives = createMap<Map<ResolvedTypeReferenceDirectiveWithFailedLookupLocations>>(); | ||
|
@@ -55,6 +62,7 @@ namespace ts { | |
resolveTypeReferenceDirectives, | ||
invalidateResolutionOfDeletedFile, | ||
invalidateResolutionOfChangedFailedLookupLocation, | ||
createHasInvalidatedResolution, | ||
clear | ||
}; | ||
|
||
|
@@ -82,6 +90,12 @@ namespace ts { | |
return collected; | ||
} | ||
|
||
function createHasInvalidatedResolution(): HasInvalidatedResolution { | ||
const collected = filesWithInvalidatedResolutions; | ||
filesWithInvalidatedResolutions = undefined; | ||
return path => collected && collected.has(path); | ||
} | ||
|
||
function resolveModuleName(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost): ResolvedModuleWithFailedLookupLocations { | ||
const primaryResult = ts.resolveModuleName(moduleName, containingFile, compilerOptions, host); | ||
// return result immediately only if it is .ts, .tsx or .d.ts | ||
|
@@ -250,7 +264,7 @@ namespace ts { | |
cache: Map<Map<T>>, | ||
getResult: (s: T) => R, | ||
getResultFileName: (result: R) => string | undefined) { | ||
cache.forEach((value, path) => { | ||
cache.forEach((value, path: Path) => { | ||
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. Same comment as above |
||
if (path === deletedFilePath) { | ||
cache.delete(path); | ||
value.forEach((resolution, name) => { | ||
|
@@ -264,6 +278,7 @@ namespace ts { | |
if (result) { | ||
if (getResultFileName(result) === deletedFilePath) { | ||
resolution.isInvalidated = true; | ||
(filesWithInvalidatedResolutions || (filesWithInvalidatedResolutions = createMap<true>())).set(path, true); | ||
} | ||
} | ||
} | ||
|
@@ -275,14 +290,13 @@ namespace ts { | |
function invalidateResolutionCacheOfChangedFailedLookupLocation<T extends NameResolutionWithFailedLookupLocations>( | ||
failedLookupLocation: string, | ||
cache: Map<Map<T>>) { | ||
cache.forEach((value, _containingFilePath) => { | ||
cache.forEach((value, containingFile: Path) => { | ||
if (value) { | ||
value.forEach((resolution, __name) => { | ||
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. Same comment as above |
||
if (resolution && !resolution.isInvalidated && contains(resolution.failedLookupLocations, failedLookupLocation)) { | ||
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. Should |
||
// TODO: mark the file as needing re-evaluation of module resolution instead of using it blindly. | ||
// Note: Right now this invalidation path is not used at all as it doesnt matter as we are anyways clearing the program, | ||
// which means all the resolutions will be discarded. | ||
// Mark the file as needing re-evaluation of module resolution instead of using it blindly. | ||
resolution.isInvalidated = true; | ||
(filesWithInvalidatedResolutions || (filesWithInvalidatedResolutions = createMap<true>())).set(containingFile, true); | ||
} | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using
any
. Use() => false
if you need a function that always returns a falsy value.