Keep projects open on tsserver via OpenExternalProject #160

lorenzodallavecchia · 2017-03-23T13:18:23Z

This change makes tsserver aware of existing Eclipse projects, so that it does not forget about project state when all files are closed.
Closes #142.

angelozerr · 2017-03-23T13:46:11Z

core/ts.core/src/ts/client/ITypeScriptServiceClient.java

@@ -91,8 +92,8 @@
 	 * @param insertString
 	 * @throws TypeScriptException
 	 */
-	void changeFile(String fileName, int line, int offset, int endLine, int endOffset, String insertString)
-			throws TypeScriptException;
+	void changeFile(String fileName, int line, int offset, int endLine, int endOffset,


Please use the same Java Format preferences (Eclipse [built-in].

I had the correct preferences configured, but somehow the format on save did not include some lines. I'm double-checking that now.

angelozerr · 2017-03-23T13:51:58Z

core/ts.core/src/ts/resources/TypeScriptProject.java

+					String projectName = projectDir.getCanonicalPath();
+					List<String> rootFiles = new ArrayList<>();
+					for (String tsconfigFilePath : getTsconfigFilePaths()) {
+						rootFiles.add(new File(projectDir, tsconfigFilePath).getCanonicalPath());


Please use FileUtils.getPath(new File(projectDir, tsconfigFilePath)) to normalize path.

angelozerr · 2017-03-23T13:54:28Z

...pse/ts.eclipse.ide.core/src/ts/eclipse/ide/internal/core/resources/IDETypeScriptProject.java

+		List<String> result = new ArrayList<>(tsconfigBuildPaths.length);
+		for (ITsconfigBuildPath tsconfigBuildPath : tsconfigBuildPaths) {
+			IFile tsconfigFile = tsconfigBuildPath.getTsconfigFile();
+			result.add(tsconfigFile.getLocation().makeRelativeTo(tsconfigFile.getProject().getLocation()).toString());


Please use WorkbenchResourceUtil.getRelativePath(tsconfigFile, tsconfigFile.getProject())

angelozerr · 2017-03-23T13:55:36Z

Wow great PR @lorenzodallavecchia! Thanks! I have added some little comments. Please fix it.

Required requests are use to have some requests waiting for the response to other requests. This would be required for openExternalProject (see angelozerr#142). Also adjusted the code so that: - async methods return CompletableFuture and never throw, - sync methods block for the future result and throw in case of error.

The server can now correctly handle files that are not open, including finding references, etc. The performance should also be better, since the project is not recreated on the server each time the "first" file is opened. Closes angelozerr#142

This is no longer needed because the server does not loose track of files. Reverts commit babde9c.

angelozerr · 2017-03-25T12:11:31Z

Thanks a lot @lorenzodallavecchia ! Very cool PR.

angelozerr · 2017-03-25T12:18:33Z

@lorenzodallavecchia now when I open file, the editor is freezing because of waitForFuture. I would like really avoid freezing editor. Could you fix that please?

avoid freeze. See #160 (comment)

angelozerr · 2017-03-25T12:25:39Z

@lorenzodallavecchia I have uncomment the use of waitForFuture to avoid freeze of editor. Don't hesitate to do PR if you find a better solution. I would like to avoid freezing the editor.

See #160

…lProject. See #160" This reverts commit 03cc7a4.

…est to avoid freeze. See #160 (comment)" This reverts commit b73e5cc.

angelozerr · 2017-03-25T15:28:48Z

@lorenzodallavecchia I'm really sorry. I have reverted your PR because the editor froze every time. And sometimes it freezees Eclipse IDE (I had to kill it).

I would like to do a release this month, so I would like to have a stable version. I would like to integrate your PR but not for 1.2.0 and study why there are so many freeze. Hope you will understand.

lorenzodallavecchia · 2017-03-27T12:12:19Z

@angelozerr don't worry: I understand.
I am perfectly fine with that feature not landing in 1.2.0, but I hope it will land eventually because the current issues with language services as well as issues #143 and #144 are preventing me from using compile on save, which is far superior to tsc-based build on save.

The freezing problem is really strange I did test opening and closing editors many times and experienced absolutely no freezing. Also, that waiting stuff should not actually change anything compared to now because:

if the caller was using CompletableFuture (the most common case), than the caller would just be waiting for a chain of two completion stages instead of one,
if the caller was using get on the future to wait for the result, it was already waiting before my change.

So, overall really really strange.
Could you provide some directions to reproduce the freezing problem?

angelozerr · 2017-03-27T21:33:55Z

@lorenzodallavecchia I will try to investigate more but my "main" project that I test is https://github.com/Microsoft/vscode which is big. For instance when I open https://github.com/Microsoft/vscode/blob/master/extensions/javascript/src/features/packageJSONContribution.ts there is a little freeze. After when I navigate quicly (must find what I do exactly), Eclipse IS freeze and I must kill it.

lorenzodallavecchia · 2017-05-22T12:12:52Z

Hi @angelozerr, I finally had time to look into this issue again.

I tried checking out https://github.com/Microsoft/vscode into a single Eclipse project. Then I opened many files (up to 60 at the same time) and navigated back and forth using Ctrl+click and outlines.
Apart from the initial wait (which I think is due to the project being large), I experienced no freezing at all.
I'm really puzzled.

Can you give me other pointers about how to reproduce the issue?

Otherwise I think I would have to remake this PR without the CompletableFuture change: it would be more fragile but still hopefully able to fix #142.

angelozerr · 2017-05-23T11:58:08Z

Hi @lorenzodallavecchia ! Glad to have your news:)

To be honnest with you, I had tested quicly, but I would like really avoid having freezing when user open an editor even if tsserver is not available. Today it works like this. The only freeze that it exists today is when user open completion, hyperlink but for other features (outline, mark occurences, etc), the editor doesn't freeze.

For completion, Oxygen should provide an async completion (hope it will resolve the freeze problem). For Hyperlink, hope Eclipse guys will fix that.

I will b every glad to have a PR, but please avoid having freeze even the first time when editor is opened. Hope you will understand my wish.

I'm very busy with other topics like autoimport with completion, integrates angular language service (inside HTML) and CodeLens, so it's difficult for me to help you with your issue. Any PR are welcome!

lorenzodallavecchia · 2017-05-23T12:11:27Z

Thanks for helping out @angelozerr.

I completely agree with the design principle of postponing waits as much as possible.
Since I was trying with smaller projects, the "first-time" wait was not present. Now that I see that, I'm am convinced too that chaining a wait before file opening is not a wise thing to do.

I think I will investigate inside tsserver to be sure that even if openExternalProject finishes after openFile, the server state remains consistent.

angelozerr · 2017-05-23T12:16:04Z

@lorenzodallavecchia I think you should study too the VSCode TypeScript client. For instance https://github.com/Microsoft/vscode/blob/master/extensions/typescript/src/typescriptServiceClient.ts#L697 which consumes ExternalProjectCompilerOptions

lorenzodallavecchia · 2017-06-05T13:00:13Z

Ok @angelozerr, I finally have a clear understanding of this problem.

First of all, VSCode is not using openExternalProject. That means the server is automatically picking up the tsconfig.json files that are closest to the edited files. Since the VSCode source is made up of many small projects, the language server is fast, since it only has to manage small projects and only a few at once (no more than the number of open files).
The downside is that there is no cross-project error checking in VSCode (exactly what I would like to have in Eclipse).

The fix I proposed in this PR was causing a slowdown when editing VSCode because it caused all TS projects to be loaded when opening just a single file. While this enables cross-project checking, it also wastes a lot of resources.
The situation was acceptable in my tests because I mostly use Eclipse projects containing only one tsconfig.json, so the cost of loading a single file is the same.

I tried removing the wait for Future values, but that only moved the problem to the subsequent tsserver calls. For example, the Outline was taking 15 seconds to appear! Ditto for the content assistant. Of course, that is not acceptable.

Unfortunately, this situation is a limitation of TypeScript and I think we would have to wait for them to implement microsoft/TypeScript#3469. That issue will introduce a way for a tsconfig.json file to reference another and have cross-project checking without impacting performance.

angelozerr · 2017-06-07T06:08:43Z

Many thank's @lorenzodallavecchia for your great feedback!

If I have understood, we must wait for microsoft/TypeScript#3469 to fix this issue.

angelozerr reviewed Mar 23, 2017

View reviewed changes

lorenzodallavecchia added 3 commits March 23, 2017 15:29

Removed forced opening of files when compiling with tsserver

d807513

This is no longer needed because the server does not loose track of files. Reverts commit babde9c.

lorenzodallavecchia force-pushed the pr/keep-open-project branch from 3db8141 to d807513 Compare March 23, 2017 14:45

angelozerr force-pushed the master branch 2 times, most recently from 302ce8b to d0129a1 Compare March 25, 2017 12:00

angelozerr added this to the 1.2.0 milestone Mar 25, 2017

angelozerr merged commit fc3d75f into angelozerr:master Mar 25, 2017

angelozerr added a commit that referenced this pull request Mar 25, 2017

Don't use waitForFuture for open, close, change tsserver request to

b73e5cc

avoid freeze. See #160 (comment)

angelozerr added a commit that referenced this pull request Mar 25, 2017

Check if TypeScript Runtime is >=2.0.5 to consume OpenExternalProject.

03cc7a4

See #160

angelozerr added a commit that referenced this pull request Mar 25, 2017

Revert "Check if TypeScript Runtime is >=2.0.5 to consume OpenExterna…

19ffac5

…lProject. See #160" This reverts commit 03cc7a4.

angelozerr added a commit that referenced this pull request Mar 25, 2017

Revert "Don't use waitForFuture for open, close, change tsserver requ…

a36b5eb

…est to avoid freeze. See #160 (comment)" This reverts commit b73e5cc.

lorenzodallavecchia mentioned this pull request Jul 26, 2017

Language services require at least one open file #142

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep projects open on tsserver via OpenExternalProject #160

Keep projects open on tsserver via OpenExternalProject #160

lorenzodallavecchia commented Mar 23, 2017

angelozerr Mar 23, 2017

lorenzodallavecchia Mar 23, 2017

angelozerr Mar 23, 2017

angelozerr Mar 23, 2017

angelozerr commented Mar 23, 2017

angelozerr commented Mar 25, 2017

angelozerr commented Mar 25, 2017

angelozerr commented Mar 25, 2017

angelozerr commented Mar 25, 2017

lorenzodallavecchia commented Mar 27, 2017

angelozerr commented Mar 27, 2017

lorenzodallavecchia commented May 22, 2017

angelozerr commented May 23, 2017

lorenzodallavecchia commented May 23, 2017

angelozerr commented May 23, 2017 •

edited

Loading

lorenzodallavecchia commented Jun 5, 2017

angelozerr commented Jun 7, 2017

Keep projects open on tsserver via OpenExternalProject #160

Keep projects open on tsserver via OpenExternalProject #160

Conversation

lorenzodallavecchia commented Mar 23, 2017

angelozerr Mar 23, 2017

Choose a reason for hiding this comment

lorenzodallavecchia Mar 23, 2017

Choose a reason for hiding this comment

angelozerr Mar 23, 2017

Choose a reason for hiding this comment

angelozerr Mar 23, 2017

Choose a reason for hiding this comment

angelozerr commented Mar 23, 2017

angelozerr commented Mar 25, 2017

angelozerr commented Mar 25, 2017

angelozerr commented Mar 25, 2017

angelozerr commented Mar 25, 2017

lorenzodallavecchia commented Mar 27, 2017

angelozerr commented Mar 27, 2017

lorenzodallavecchia commented May 22, 2017

angelozerr commented May 23, 2017

lorenzodallavecchia commented May 23, 2017

angelozerr commented May 23, 2017 • edited Loading

lorenzodallavecchia commented Jun 5, 2017

angelozerr commented Jun 7, 2017

angelozerr commented May 23, 2017 •

edited

Loading