Add existed java extensions into jdt.ls initialization parameters #2593

svor · 2018-08-16T13:23:29Z

Signed-off-by: Valeriy Svydenko [email protected]

What does this PR do:

This PR provides an ability to find all plugins(bundles) of jdt.ls in Theia's extensions and add them into initializationOptions when Java Language Server start initializing.

Reference issues

#1878

akosyakov · 2018-08-16T14:40:11Z

packages/java/src/browser/java-client-contribution.ts

@@ -111,9 +107,25 @@ export class JavaClientContribution extends BaseLanguageClientContribution {
        });
    }

-    protected createOptions(): LanguageClientOptions {
+    async activate(): Promise<Disposable> {


please make sure to call super.activate to make sure that restarting works properly, it was changed recently

akosyakov · 2018-08-16T14:44:27Z

packages/java/src/common/java-common.ts

@@ -0,0 +1,38 @@
+/********************************************************************************
+ * Copyright (C) 2017 TypeFox and others.


please check new copyrights, that it is 2018 and Red Hat for you

akosyakov · 2018-08-16T14:46:13Z

packages/java/src/node/java-service.ts

+ */
+@injectable()
+export class JavaServiceImpl implements JavaService {
+    async collectJavaExtensions(): Promise<string[]> {


could be collected just one, there is no way to change extensions without restart

akosyakov · 2018-08-16T14:48:00Z

packages/languages/src/browser/language-client-contribution.ts

                if (toDeactivate.disposed) {
                    messageConnection.dispose();
                    return;
                }
                toDeactivate.push(messageConnection);

-                const languageClient = this.createLanguageClient(messageConnection);
+                const languageClient = await this.createLanguageClient(messageConnection);


if a connection is disposed in the meantime then we create a language client for a broken connection. I don't think this code can be async. Would it be possible to prefetch java extension as part of waitForActivation only once?

akosyakov · 2018-08-17T05:15:26Z

packages/java/src/browser/java-client-contribution.ts

+    }
+
+    waitForActivation(app: FrontendApplication): Promise<void> {
+        return Promise.race([super.waitForActivation(app), this.javaService.collectJavaExtensions().then(extensions => {


Promise.race resolves if one of given promises is resolved, you want Promise.all or simpler:

await super.waitForActivation(app); this.javaExtensions = await this.javaServices.collectJavaExtensions();

Fixed, thank you

akosyakov · 2018-08-17T05:15:50Z

packages/java/src/browser/java-client-contribution.ts

@@ -42,8 +43,10 @@ export class JavaClientContribution extends BaseLanguageClientContribution {
    readonly name = JAVA_LANGUAGE_NAME;
    private readonly statusNotificationName = 'java-status-notification';
    private statusBarTimeout: number | undefined;
+    private javaExtensions: string[];


(nit) string[] | undefined if something is not initialized in the contructor

akosyakov · 2018-08-17T05:16:11Z

packages/java/src/node/java-service.ts

+ */
+@injectable()
+export class JavaServiceImpl implements JavaService {
+    private javaExtensions: string[];


(nit) string[] | undefined if something is not initialized in the constructor

akosyakov · 2018-08-17T05:17:18Z

packages/java/src/node/java-service.ts

+        }
+    }
+
+    dispose(): void {


JavaService don't need to be Disposable or a method should not do anything, otherwise, it can throw if someone call dispose

fixed, thank you!

svor · 2018-08-17T09:06:20Z

Hi @akosyakov
Thank you for the reviews!
I've applied fixes, take please another look at it when you have some free time.
Also I've created a new issue for searching java-extensions in Theia plugins: #2600.

akosyakov

LGTM

tolusha · 2018-08-17T09:17:26Z

packages/java/src/common/java-common.ts

+     * Finds all java extensions which are provided in extensions.
+     * @return an array of paths to the plugins
+     */
+    collectJavaExtensions(): Promise<string[] | undefined>;


What is the purpose to return undefined ? It is better to return an empty array if there are no extensions.

The javadoc is not clear. Does it return extensions or plugins?

"collectExtensionBundleIds()" perhaps? Or is it the path to the bundle jar? A more specific name would help.

It is a path to the bundle jar, I think collectExtensionBundlesPaths() would be better

akosyakov · 2018-08-17T09:23:34Z

@svor before merging, please clean up history, that we don't have commits like addressed comments or code cleanup

tsmaeder · 2018-08-17T09:30:12Z

packages/java/src/browser/java-client-contribution.ts

    protected createOptions(): LanguageClientOptions {
        const options = super.createOptions();
        options.initializationOptions = {
+            bundles: this.javaExtensions,


If I understand correctly, the only reason to have a front-end contribution at all is to populate this field. In my mind, it would be nicer to intercept the init message on the back end and to populate the field there. If possible, we should not let these implementation details dictate architecture.

Yes, you understand correctly. Is there a way to intercept the init command on the server?

https://github.com/theia-ide/theia/blob/cc6d2d3b2b409c0d92a33661c2b9272c992461f3/packages/languages/src/node/language-server-contribution.ts#L62

you can override it in https://github.com/theia-ide/theia/blob/master/packages/java/src/node/java-contribution.ts

please make sure to use the super implementation as well

Thanks @akosyakov. Moved to the server side.

tsmaeder · 2018-08-17T09:31:44Z

packages/java/src/common/java-common.ts

+     * Finds all java extensions which are provided in extensions.
+     * @return an array of paths to the plugins
+     */
+    collectJavaExtensions(): Promise<string[] | undefined>;


"collectExtensionBundleIds()" perhaps? Or is it the path to the bundle jar? A more specific name would help.

tsmaeder · 2018-08-17T09:42:31Z

packages/java/src/node/java-service.ts

+        if (!this.javaExtensions) {
+            this.javaExtensions = [];
+        }
+        const contributes: JavaContributes = extPck.raw.contributes;


Sorry, but I don't like this part at all. We should not be nosing around in other extensions package.json. We should be defining an extension interface and let the contributing extensions decide how the want to give us a list of extension jars. Like

interface JdtLsExtension { getExtensionJars() : String }

We can still use package.json for a default implementation that contributors can use.

I still don't understand why declaration in package.json is bad idea. As for me it is simpler to add a path to the my bundle into packaje.json instead of implement some interface and it will work for both: extensions and plugins in Theia.

tsmaeder · 2018-08-17T09:43:17Z

packages/java/src/node/java-service.ts

+import * as paths from 'path';
+
+interface JavaContributes extends NodePackage {
+    javaExtensions?: string[];


This needs documentation. How does this work? I assume I need to add some text to package.json? What is the syntax of the strings?

changed README

tsmaeder · 2018-08-22T12:28:44Z

packages/java/src/node/java-util.ts

+            return this.javaExtensions;
+        }
+
+        const projectPath = process.cwd();


As I understand this code, in the Java bundle, I would have to declare other bundles who can extend jdt.ls? This is not what we want, we want to have unknown third party bundles contribute bundles to jdt.ls. We should not be reading other extensions package.json by hand, we should declare an extension point, which other extensions can contribute to and then have the system pass us those extensions.

@tsmaeder it's not that. You have not declare bundles in Java extension if you want to extend jdt.ls.
Java extension in Theia just contains a mechanism for scanning all other Theia extensions and if some of them has java bundle (plugin for jdt.ls) it takes a path to this bundle (which was described in packaje.json in this extension) and adds this path to initializationOptions. And when jdt.ls is starting all found bundles from Theia extensions will be installed as well.

Imagine this:

Theia has A extension.

This extension contains bundle (custom-java-plugin.jar) to jdt.ls (in some subfolder)

package.json in A contains the path to the bundle

So, what Java extension does:

Before initializing jdt.ls Java extension scans all other Theia extensions

It finds that extension A has declaration about jdt.ls plugin (custom-java-plugin.jar)

Builds absolute path to this plugin (/..../A/subfolder/custom-java-plugin.jar)

Adds this path to initializationOptions and initializes jdt.ls

jdt.ls loads custom-java-plugin.jar bundle and installs it.

akosyakov · 2018-09-14T12:00:49Z

packages/java/src/node/java-util.ts

+        }
+
+        const projectPath = process.cwd();
+        const manager = new ApplicationPackageManager({ projectPath });


@svor I am not sure about the state of this PR, but if you continue, please use ApplicationPackage instead and inject it as here: https://github.com/theia-ide/theia/blob/b57e11f1f9635b36efdc5a8478248d26e3a62035/packages/typescript/src/node/typescript-contribution.ts#L33-L49

ApplicationPackageManager brings dev time dependencies, like webpack. We don't want to have them at runtime.

@akosyakov I'm going to continue my work on this PR, thank you!

svor · 2018-09-17T13:12:10Z

@tsmaeder @akosyakov I have another variant to solve current problem, can you please review my last commit.

I've reworked this PR to provide an interface which should be implemented by contributing extensions.
Only one methods in this interface which returns list of paths to jd.ls bundles.

Just one question here: is it possible to get absolute path to the extension to build absolute path of jdt.ls bundle which included into this extension.

akosyakov · 2018-09-17T13:25:38Z

packages/java/src/node/java-extension-model.ts

@@ -0,0 +1,31 @@
+/********************************************************************************
+ * Copyright (C) 2017 TypeFox and others.


Red Hat, 2018

tsmaeder

I like this version a lot better. Also shorter, no?

packages/java/src/node/java-contribution.ts

tsmaeder · 2018-09-17T18:29:22Z

packages/java/src/node/java-extension-model.ts

+ */
+export interface JavaExtensionContribution {
+    /**
+     * Returns an array of paths to the java bundles.


It might be clearer if you say "bundle jar files"

tsmaeder · 2018-09-17T18:29:49Z

packages/java/src/node/java-extension-model.ts

+export const JavaExtensionContribution = Symbol('JavaExtensionContribution');
+
+/**
+ * A contribution point for java extensions.


"..for extensions to jdt.ls"

…eters Signed-off-by: Valeriy Svydenko <[email protected]>

svor changed the title ~~Add existed java extensions into jdt.ls initialization parameters~~ [WIP] Add existed java extensions into jdt.ls initialization parameters Aug 16, 2018

akosyakov reviewed Aug 16, 2018

View reviewed changes

svor changed the title ~~[WIP] Add existed java extensions into jdt.ls initialization parameters~~ Add existed java extensions into jdt.ls initialization parameters Aug 16, 2018

akosyakov reviewed Aug 17, 2018

View reviewed changes

svor mentioned this pull request Aug 17, 2018

[Java] Find JDT LS extensions in plugins #2600

Closed

akosyakov approved these changes Aug 17, 2018

View reviewed changes

tolusha reviewed Aug 17, 2018

View reviewed changes

tsmaeder requested changes Aug 17, 2018

View reviewed changes

tsmaeder requested changes Aug 22, 2018

View reviewed changes

tsmaeder mentioned this pull request Sep 11, 2018

Languages Sprint 155 eclipse-che/che#11111

Closed

19 tasks

akosyakov reviewed Sep 14, 2018

View reviewed changes

akosyakov reviewed Sep 17, 2018

View reviewed changes

tsmaeder reviewed Sep 17, 2018

View reviewed changes

Add java bundles from the extensions into jdt.ls initialization param…

9aafc3e

…eters Signed-off-by: Valeriy Svydenko <[email protected]>

tsmaeder approved these changes Sep 18, 2018

View reviewed changes

tsmaeder merged commit 4788b6e into eclipse-theia:master Sep 20, 2018

tsmaeder mentioned this pull request Oct 2, 2018

Languages Sprint 156 (K-1) eclipse-che/che#11438

Closed

15 tasks

		@@ -0,0 +1,38 @@
		/********************************************************************************
		* Copyright (C) 2017 TypeFox and others.

		@@ -0,0 +1,31 @@
		/********************************************************************************
		* Copyright (C) 2017 TypeFox and others.

Add existed java extensions into jdt.ls initialization parameters #2593

Add existed java extensions into jdt.ls initialization parameters #2593

Conversation

svor commented Aug 16, 2018 • edited Loading

What does this PR do:

Reference issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svor commented Aug 17, 2018

akosyakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolusha Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svor Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

akosyakov commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svor Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

akosyakov Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svor commented Sep 17, 2018

Choose a reason for hiding this comment

tsmaeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svor commented Aug 16, 2018 •

edited

Loading

tolusha Aug 17, 2018 •

edited

Loading

svor Aug 17, 2018 •

edited

Loading

akosyakov Aug 17, 2018 •

edited

Loading

svor Aug 22, 2018 •

edited

Loading

akosyakov Sep 14, 2018 •

edited

Loading