-
Notifications
You must be signed in to change notification settings - Fork 73
Add typeroots support #343
Changes from 8 commits
ed8052b
e0addb0
5a10ad3
c520bb2
e19ffc2
5cf7be3
1d75073
c259701
3473993
1422457
08eec1c
521eba0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -321,6 +321,7 @@ export class ProjectManager implements Disposable { | |
span.addTags({ uri, maxDepth }); | ||
ignore.add(uri); | ||
return this.ensureModuleStructure(span) | ||
.concat(Observable.defer(() => this.ensureConfigDependencies())) | ||
// If max depth was reached, don't go any further | ||
.concat(Observable.defer(() => maxDepth === 0 ? Observable.empty<never>() : this.resolveReferencedFiles(uri))) | ||
// Prevent cycles | ||
|
@@ -337,6 +338,24 @@ export class ProjectManager implements Disposable { | |
}); | ||
} | ||
|
||
isConfigDependency(filePath: string): 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. please add a docblock |
||
for (const config of this.configurations()) { | ||
config.ensureConfigFile(); | ||
if (config.isExpectedDeclarationFile(filePath)) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
ensureConfigDependencies(): Observable<never> { | ||
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. please add a docblock |
||
return observableFromIterable(this.inMemoryFs.uris()) | ||
.filter(uri => this.isConfigDependency(uri2path(uri))) | ||
.mergeMap(uri => this.updater.ensure(uri)) | ||
.publishReplay() | ||
.refCount(); | ||
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. why publishReplay + refCount? 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 looked at ensureModuleStructure, which I also understood would be invoked many times but should always return the same (cached) sequence. But I'd love to know how this use case differs (and remove it if unnecessary). 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 doesn't actually save the (i.e. cache) the Observable, so it doesn't has a use here. If you want to always return the same Observable you need to save it in a property |
||
} | ||
|
||
/** | ||
* Invalidates a cache entry for `resolveReferencedFiles` (e.g. because the file changed) | ||
* | ||
|
@@ -734,6 +753,11 @@ export class ProjectConfiguration { | |
*/ | ||
private expectedFilePaths = new Set<string>(); | ||
|
||
/** | ||
* List of resolved extra root directories to allow global type declaration files to be loaded from. | ||
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. Is this restricted to global typings? 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. From https://www.typescriptlang.org/docs/handbook/tsconfig-json.html:
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. They clarify in that page as well:
|
||
*/ | ||
private typeRoots: string[]; | ||
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. is the order important? if not, use a Set 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 sure if |
||
|
||
/** | ||
* @param fs file system to use | ||
* @param documentRegistry Shared DocumentRegistry that manages SourceFile objects | ||
|
@@ -838,6 +862,11 @@ export class ProjectConfiguration { | |
this.expectedFilePaths = new Set(configParseResult.fileNames); | ||
|
||
const options = configParseResult.options; | ||
const pathResolver = /^[a-z]:\//i.test(base) ? path.win32 : path.posix; | ||
this.typeRoots = options.typeRoots ? | ||
options.typeRoots.map((r: string) => pathResolver.resolve(this.rootFilePath, r)) : | ||
[]; | ||
|
||
if (/(^|\/)jsconfig\.json$/.test(this.configFilePath)) { | ||
options.allowJs = true; | ||
} | ||
|
@@ -864,6 +893,19 @@ export class ProjectConfiguration { | |
|
||
private ensuredBasicFiles = false; | ||
|
||
/** | ||
* Determines if a fileName is a declaration file within expected files or type roots | ||
* @param fileName | ||
*/ | ||
public isExpectedDeclarationFile(fileName: string) { | ||
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. return type |
||
if (isDeclarationFile(fileName)) { | ||
return this.expectedFilePaths.has(toUnixPath(fileName)) || | ||
this.typeRoots.some(root => fileName.startsWith(root)); | ||
} else { | ||
return false; | ||
} | ||
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 function just needs to be a single boolean |
||
} | ||
|
||
/** | ||
* Ensures we added basic files (global TS files, dependencies, declarations) | ||
*/ | ||
|
@@ -882,7 +924,8 @@ export class ProjectConfiguration { | |
// Add all global declaration files from the workspace and all declarations from the project | ||
for (const uri of this.fs.uris()) { | ||
const fileName = uri2path(uri); | ||
if (isGlobalTSFile(fileName) || (isDeclarationFile(fileName) && this.expectedFilePaths.has(toUnixPath(fileName)))) { | ||
if (isGlobalTSFile(fileName) || | ||
this.isExpectedDeclarationFile(fileName)) { | ||
const sourceFile = program.getSourceFile(fileName); | ||
if (!sourceFile) { | ||
this.getHost().addFile(fileName); | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
Does this have a performance impact? I think it probably has, as you are iterating in
ensureConfigDependencies()
over every single file in the workspace, andensureReferencedFiles()
is called recursively for every referenced file, so you are repeating that iteration every time. I thinkensureConfigDependencies()
needs to memoize its return value in the same wayensureAllFiles()
does, or you could just add that logic toensureModuleStructure()
(which would already is memoized, has memoize cache invalidation logic and already iterates over all files so you'd only have to iterate once). Optionally, you could try this then on a very big TS project and see if it degrades perf in any way