Move theia remote extension there and align it with new theia 0.3.19 #42
Conversation
benoitf
requested review from
evidolob,
AndrienkoAleksandr,
mmorhun and
akurinnoy
| this.endpoints = endpointKeys.map(key => process.env[key] || ''); | ||
| this.logger.info('Plugins Endpoints are ', this.endpoints); | ||
|
|
||
| const pluginEndpointKeys: string[] = Object.keys(process.env).filter(key => key.startsWith(HostedPluginMapping.ENDPOINT_ENV_VAR_PREFIX)); |
There was a problem hiding this comment.
Why do you need to grab end points second time here?
There was a problem hiding this comment.
good catch (again)
I think it was a bad refactoring at some time
code was like that in current version
https://github.com/ws-skeleton/che-theia-remote-extension/blob/master/src/node/hosted-plugin-remote.ts#L52-L60
will update, thx
| * @author Florent Benoit | ||
| */ | ||
| @injectable() | ||
| export class HostedPluginMapping { |
There was a problem hiding this comment.
To me this name is very confusing. AFAIK we call hosted an instance of Theia which hosts plug-in under development. But here we are talking about remote plug-ins which has nothing in common with a plug-in development process. To me RemotePluginMapping works fine. Or maybe I misunderstood something?
There was a problem hiding this comment.
it's part of the instance of theia which hosts plug-in: pluginHost is the name of what is used to host a plugin ?
There was a problem hiding this comment.
Then every instance of Theia is hosted, because it has plugins.
There was a problem hiding this comment.
But I see what you wanted to say. Thanks for explanation.
| setupWebsocket(): void { | ||
| this.hostedPluginMapping.getEndPoints().forEach(endpointAdress => { | ||
| if (endpointAdress) { | ||
| const websocket = new ws(endpointAdress); |
There was a problem hiding this comment.
Minor remark: new ws(...) looks strange a bit in terms of naming conventions. WebSocket ?
The same with new Map<string, ws>();. At first glance I thought it is a variabale, not a type...
There was a problem hiding this comment.
we do
import * as ws from 'ws';
so you want to change the name of the module ?
There was a problem hiding this comment.
Well, I think it will be better in this particular case (it is local name anyway).
| * @author Florent Benoit | ||
| */ | ||
| @injectable() | ||
| export class HostedPluginRemote { |
There was a problem hiding this comment.
Is it for hosted plugin only?
| console.log(`Theia Endpoint ${process.pid}/pid listening on port`, PLUGIN_PORT); | ||
| const webSocketServer = new WebSocketServerImpl({ port: PLUGIN_PORT }); | ||
|
|
||
| interface CheckAliveWS extends ws { |
There was a problem hiding this comment.
Please group declaration and commands.
Or would be nice to put all the entrypoint commands into separate function.
There was a problem hiding this comment.
ok will check. For info it was like that already in the other repository.
There was a problem hiding this comment.
moved console.log command at the end of the file
| return socket.terminate(); | ||
| } | ||
| socket.alive = false; | ||
| socket.ping(); |
There was a problem hiding this comment.
I think we should retry a few times in case of short network outage.
…0.3.19 upstream changes Change-Id: I13d36ee1a69169f4745da88dfbe081f627b233a9 Signed-off-by: Florent Benoit <fbenoit@redhat.com>
benoitf
force-pushed
the
eclipse-che-theia-remote
branch
from
caa3822
to
d8272a7
Compare
…0.3.19 upstream changes Change-Id: I704a47e1039b4cc5c5c76f8986e1d1471314d4c9 Signed-off-by: Florent Benoit <fbenoit@redhat.com>
…0.3.19 upstream changes Change-Id: I9e30a913d403eaf64761d90815c84199ad60372d Signed-off-by: Florent Benoit <fbenoit@redhat.com>
Move extension from https://github.com/ws-skeleton/che-theia-remote-extension
start enhancements for eclipse-che/che#12392 and eclipse-che/che#12292