Refactor and cleanup GLSPContribution API #146
Conversation
tortmayr
force-pushed
the
tortmayr/issues/848
branch
from
0564786 to
ad1346f
Compare
Drop dependency to sprotty-theia , move dependent code in-repo and adapt existing code base. Code that is adapted from the orginal sprotty-theia repository is proplery marked in the copyright notice and with an inline comment Depends on: #146 Fixes eclipse-glsp/glsp#847
Drop dependency to sprotty-theia , move dependent code in-repo and adapt existing code base. Code that is adapted from the orginal sprotty-theia repository is proplery marked in the copyright notice and with an inline comment Depends on: #146 Fixes eclipse-glsp/glsp#847
| if (data) { | ||
| const message = data.toString(); | ||
| protected override processLogInfo(line: string): void { | ||
| if (line) { |
There was a problem hiding this comment.
Can line be undefined? If so, the method signature should actually show this. If not then I do not see any advantage of checking for empty string here if we check for startsWith anyway.
| const message = data.toString(); | ||
| protected override processLogInfo(line: string): void { | ||
| if (line) { | ||
| const message = line.toString(); |
There was a problem hiding this comment.
Unnecessary if line really can be only of type string and not of type Buffer as available before.
| if (data) { | ||
| console.error(`${this.id}: ${data}`); | ||
| protected processLogError(line: string): void { | ||
| if (line) { |
There was a problem hiding this comment.
Do we do this to filter empty lines or because line can be undefined?
| * @returns the corresponding `GLSPClient` or `undefined` if no client is configured for the given type | ||
| */ | ||
| async getGLSPClient(contributionId: string): Promise<GLSPClient | undefined> { | ||
| return this.getGLSPClientContribution(contributionId)?.glspClient ?? undefined; |
There was a problem hiding this comment.
| return this.getGLSPClientContribution(contributionId)?.glspClient ?? undefined; | |
| return this.getGLSPClientContribution(contributionId)?.glspClient; |
| @@ -83,84 +109,58 @@ export abstract class BaseGLSPClientContribution implements GLSPClientContributi | |||
| }); | |||
| } | |||
|
|
|||
| waitForActivation(app: FrontendApplication): Promise<any> { | |||
| const activationPromises: Promise<any>[] = []; | |||
There was a problem hiding this comment.
I really like that we no longer have a default activation condition! We just need to make sure we properly document it as it definitely may break some assumptions.
|
|
||
| activate(): Disposable { | ||
| if (this.toDeactivate.disposed) { | ||
| activate(app: FrontendApplication): void { |
There was a problem hiding this comment.
Should activate maybe return a promise? Seems like we should have one at hand and it might be useful for some cases.
| } | ||
|
|
||
| deactivate(_app: FrontendApplication): void { | ||
| this.toDeactivate.dispose(); | ||
| this.toDispose.dispose(); |
There was a problem hiding this comment.
Should that call dispose or the other way around?
| }) | ||
| ]); | ||
| } | ||
| }, | ||
| { reconnecting: false } | ||
| { reconnecting: true } |
There was a problem hiding this comment.
Do we properly handle reconnection or does that work out of the box? Or does it not matter at all?
- Refactor `GLSPCLientProvider` and reuse it in `BaseGLSPTheiaConnector` - Make GLSPContributions disposable - Cleanup redundant session concept in `GLSPBackendContribution` - Cleanup & refactor`GLSPClientContribution` API. - Cleanup & refactor`GLSPSocketServerContribution` API - Remove remains from @theia/languages - Simplify client initialization by using Theia's Deferred concept. Fixes eclipse-glsp/glsp#848
tortmayr
force-pushed
the
tortmayr/issues/848
branch
from
ad1346f to
18351b7
Compare
|
Thanks for the fast review, I have added a commit that should address the open issues |
Drop dependency to sprotty-theia , move dependent code in-repo and adapt existing code base. Code that is adapted from the orginal sprotty-theia repository is proplery marked in the copyright notice and with an inline comment Depends on: #146 Fixes eclipse-glsp/glsp#847
Drop dependency to sprotty-theia , move dependent code in-repo and adapt existing code base. Code that is adapted from the orginal sprotty-theia repository is proplery marked in the copyright notice and with an inline comment Depends on: #146 Fixes eclipse-glsp/glsp#847
GLSPCLientProviderand reuse it inBaseGLSPTheiaConnectorGLSPBackendContributionGLSPClientContributionAPI.GLSPSocketServerContributionAPIFixes eclipse-glsp/glsp#848