Skip to content
This repository was archived by the owner on Oct 16, 2020. It is now read-only.

Add typeroots support #343

Merged
merged 12 commits into from
Sep 27, 2017
Merged
45 changes: 44 additions & 1 deletion src/project-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Copy link
Contributor

@felixfbecker felixfbecker Sep 20, 2017

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, and ensureReferencedFiles() is called recursively for every referenced file, so you are repeating that iteration every time. I think ensureConfigDependencies() needs to memoize its return value in the same way ensureAllFiles() does, or you could just add that logic to ensureModuleStructure() (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

// If max depth was reached, don't go any further
.concat(Observable.defer(() => maxDepth === 0 ? Observable.empty<never>() : this.resolveReferencedFiles(uri)))
// Prevent cycles
Expand All @@ -337,6 +338,24 @@ export class ProjectManager implements Disposable {
});
}

isConfigDependency(filePath: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The 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> {
Copy link
Contributor

Choose a reason for hiding this comment

The 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();
Copy link
Contributor

Choose a reason for hiding this comment

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

why publishReplay + refCount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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).

Copy link
Contributor

Choose a reason for hiding this comment

The 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)
*
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this restricted to global typings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From https://www.typescriptlang.org/docs/handbook/tsconfig-json.html:

By default all visible “@types” packages are included in your compilation.
If typesRoots is specified, only packages under typeRoots will be included.
Keep in mind that automatic inclusion is only important if you’re using files with global declarations (as opposed to files declared as modules)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They clarify in that page as well:

If you use an import "foo" statement, for instance, TypeScript may still look through node_modules & node_modules/@types folders to find the foo package.

*/
private typeRoots: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

is the order important? if not, use a Set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this.typeRoots.some(root => fileName.startsWith(root)); is a good fit for Set?


/**
* @param fs file system to use
* @param documentRegistry Shared DocumentRegistry that manages SourceFile objects
Expand Down Expand Up @@ -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;
}
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this function just needs to be a single boolean return

}

/**
* Ensures we added basic files (global TS files, dependencies, declarations)
*/
Expand All @@ -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);
Expand Down
20 changes: 20 additions & 0 deletions src/test/project-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,26 @@ describe('ProjectManager', () => {
assert.isDefined(configs.find(config => config.configFilePath === '/foo/tsconfig.json'));
});

describe('ensureBasicFiles', () => {
beforeEach(async () => {
memfs = new InMemoryFileSystem('/');
const localfs = new MapFileSystem(new Map([
['file:///project/package.json', '{"name": "package-name-1"}'],
['file:///project/tsconfig.json', '{ "compilerOptions": { "typeRoots": ["../types"]} }'],
['file:///project/file.ts', 'console.log(GLOBALCONSTANT);'],
['file:///types/types.d.ts', 'declare var GLOBALCONSTANT=1;']

]));
const updater = new FileSystemUpdater(localfs, memfs);
projectManager = new ProjectManager('/', memfs, updater, true);
});
it('loads files from typeRoots', async () => {
await projectManager.ensureReferencedFiles('file:///project/file.ts').toPromise();
memfs.getContent('file:///project/file.ts');
memfs.getContent('file:///types/types.d.ts');
});
});

describe('getPackageName()', () => {
beforeEach(async () => {
memfs = new InMemoryFileSystem('/');
Expand Down