Add typeroots support #343

tomv564 · 2017-08-29T06:44:31Z

On codebases that rely on type roots for distributing extra declaration files, this code should prevent diagnostics about missing types and improve completions/hovers/etc.

To do:

Fix lint error
Add test(s)

codecov · 2017-08-31T06:43:04Z

Codecov Report

Merging #343 into master will increase coverage by 0.32%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage    60.2%   60.52%   +0.32%     
==========================================
  Files          14       14              
  Lines        2151     2171      +20     
  Branches      351      355       +4     
==========================================
+ Hits         1295     1314      +19     
- Misses        706      707       +1     
  Partials      150      150

Impacted Files	Coverage Δ
src/project-manager.ts	`83.97% <90.47%> (+0.75%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6087974...521eba0. Read the comment docs.

tomv564 · 2017-08-31T06:47:15Z

@felixfbecker, I am a bit stuck on this test - the types.d.ts file is added to the in-memory file system but its content is not loaded. What am I missing?

tomv564 · 2017-09-08T06:44:32Z

@felixfbecker The content-preload for declaration files was missing typeRoots, tests pass with windows and unix-style paths now.
I'm not happy with having introduced yet another ensure function, but couldn't find any other place that loads content based on tsconfig settings?

felixfbecker · 2017-09-08T10:06:12Z

src/project-manager.ts

+			.filter(uri => this.isConfigDependency(uri2path(uri)))
+			.mergeMap(uri => this.updater.ensure(uri))
+			.publishReplay()
+			.refCount();


why publishReplay + refCount?

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

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

felixfbecker · 2017-09-08T10:06:32Z

src/project-manager.ts

@@ -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;


the regexp should only match at the beginning of the string

felixfbecker · 2017-09-08T10:06:42Z

src/project-manager.ts

@@ -865,6 +894,19 @@ export class ProjectConfiguration {
 	private ensuredBasicFiles = false;

 	/**
+	 * [isExpectedDeclarationFile description]
+	 * @param {string} fileName [description]


…plate

tomv564 · 2017-09-15T18:18:59Z

@felixfbecker: would you have time to look at this PR again soon?

felixfbecker · 2017-09-15T18:25:48Z

src/project-manager.ts

@@ -735,6 +754,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.


Is this restricted to global typings?

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)

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.

felixfbecker · 2017-09-15T18:26:54Z

src/project-manager.ts

@@ -735,6 +754,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.
+	 */
+	private typeRoots: string[];


is the order important? if not, use a Set

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

felixfbecker · 2017-09-15T18:27:31Z

src/project-manager.ts

+					this.typeRoots.some(root => fileName.startsWith(root));
+		} else {
+			return false;
+		}


this function just needs to be a single boolean return

felixfbecker · 2017-09-15T18:27:38Z

src/project-manager.ts

+	 * Determines if a fileName is a declaration file within expected files or type roots
+	 * @param fileName
+	 */
+	public isExpectedDeclarationFile(fileName: string) {


return type

felixfbecker · 2017-09-15T18:28:40Z

src/project-manager.ts

@@ -337,6 +338,24 @@ export class ProjectManager implements Disposable {
 		});
 	}

+	isConfigDependency(filePath: string): boolean {


please add a docblock

felixfbecker · 2017-09-15T18:28:49Z

src/project-manager.ts

+		return false;
+	}
+
+	ensureConfigDependencies(): Observable<never> {


please add a docblock

felixfbecker

This looks almost good to me

tomv564 · 2017-09-16T21:31:02Z

@felixfbecker: your suggestions have been applied (with the exception of Set, see comment)

tomv564 · 2017-09-20T20:24:22Z

Ping @felixfbecker, congrats on the big release today. Any chance we can work towards merging this PR and the plugin support PR?

felixfbecker · 2017-09-20T21:11:46Z

Thank you, as you can imagine I was very busy in the last days :) Taking a look

felixfbecker · 2017-09-20T21:17:51Z

src/project-manager.ts

@@ -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()))


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

tomv564 · 2017-09-25T20:21:42Z

@felixfbecker I applied your suggestion, and verified that:

The logic still works with a typeRoots dependent project
Hovers on the Microsoft/Typescript source are still appearing quickly

felixfbecker reviewed Sep 8, 2017

View reviewed changes

Tom van Ommeren and others added 8 commits September 9, 2017 11:48

read and resolve typeRoots so ensureBasicFiles can add content

ed8052b

fix: clean up ensureBasicFiles logic

e0addb0

fix: Lint fix

5a10ad3

fix: Add (broken) test

c520bb2

fix: Add ensureConfigDependencies to fix test

e19ffc2

fix: variable naming and windows slashes

5cf7be3

fix: typeroot resolving on windows

1d75073

fix: Check for drive prefix at start of string only, complete doc tem…

c259701

…plate

tomv564 force-pushed the add-typeroots-support branch from 31c44a0 to c259701 Compare September 9, 2017 11:20

felixfbecker reviewed Sep 15, 2017

View reviewed changes

src/project-manager.ts Outdated

return false;

}

ensureConfigDependencies(): Observable<never> {

Copy link

Contributor

felixfbecker Sep 15, 2017

Choose a reason for hiding this comment

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

please add a docblock

felixfbecker reviewed Sep 15, 2017

View reviewed changes

fix: Add doctypes, remove publishReplay&refCount, simplify

3473993

felixfbecker reviewed Sep 20, 2017

View reviewed changes

fix: memoize ensureConfigDependencies

1422457

tomv564 and others added 2 commits September 26, 2017 08:03

fix: add ts2.4 compile fix to ensureConfigDependencies

08eec1c

Merge branch 'master' into add-typeroots-support

521eba0

felixfbecker approved these changes Sep 27, 2017

View reviewed changes

felixfbecker merged commit 25e0108 into sourcegraph:master Sep 27, 2017

tomv564 deleted the add-typeroots-support branch September 27, 2017 05:48

Add typeroots support #343

Add typeroots support #343

Uh oh!

Conversation

tomv564 commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tomv564 commented Aug 31, 2017

Uh oh!

tomv564 commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomv564 commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfbecker left a comment

Choose a reason for hiding this comment

Uh oh!

tomv564 commented Sep 16, 2017

Uh oh!

tomv564 commented Sep 20, 2017

Uh oh!

felixfbecker commented Sep 20, 2017

Uh oh!

felixfbecker Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomv564 commented Sep 25, 2017

Uh oh!

Uh oh!

tomv564 commented Aug 29, 2017 •

edited

Loading

codecov bot commented Aug 31, 2017 •

edited

Loading

felixfbecker Sep 20, 2017 •

edited

Loading