-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add websocket server launcher #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much, Tobias! Works great for me for the most part (just a minor package.json error) but I do have some comments on the code which we maybe want to address.
@@ -12,41 +12,6 @@ The server consists of three components: | |||
The main target environment is node, nevertheless, all components are implemented in an ismorphic fashion and also provide | |||
an entrypoint to target browser environments (e.g. running the server in a web worker) | |||
|
|||
## Integration & Features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add some information about the websocket server in ## Start & Debug
?
package.json
Outdated
@@ -20,6 +20,7 @@ | |||
"publish:next": "SHA=$(git rev-parse --short HEAD) && lerna publish preminor --exact --canary --preid next.${SHA} --dist-tag next --no-git-reset --no-git-tag-version --no-push --ignore-scripts --yes --no-verify-access", | |||
"publish:prepare": "lerna version --ignore-scripts --yes --no-push", | |||
"start": "yarn --cwd examples/workflow-server start", | |||
"start:websocket": "yarn --cwd examples/workflow-server start;websocket", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"start:websocket": "yarn --cwd examples/workflow-server start;websocket", | |
"start:websocket": "yarn --cwd examples/workflow-server start:websocket", |
|
||
const START_UP_COMPLETE_MSG = '[GLSP-Server]:Startup completed.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const START_UP_COMPLETE_MSG = '[GLSP-Server]:Startup completed.'; | |
export const START_UP_COMPLETE_MSG = '[GLSP-Server]:Startup completed.'; | |
@@ -20,7 +20,7 @@ import { GLSPServerLauncher } from '../../common/launch/glsp-server-launcher'; | |||
import { GLSPServer, JsonRpcGLSPServer } from '../../common/protocol/glsp-server'; | |||
import { Logger } from '../../common/utils/logger'; | |||
|
|||
const START_UP_COMPLETE_MSG = '[GLSP-Server]:Startup completed. Accepting requests on port:'; | |||
export const START_UP_COMPLETE_MSG = '[GLSP-Server]:Startup completed. Accepting requests on port:'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const START_UP_COMPLETE_MSG = '[GLSP-Server]:Startup completed. Accepting requests on port:'; | |
export const START_UP_COMPLETE_MSG = '[GLSP-Server]:Startup completed. Accepting requests on port:'; | |
server.setTimeout(0); | ||
return server; | ||
} | ||
protected stop(): MaybePromise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protected stop(): MaybePromise<void> { | |
protected stop(): MaybePromise<void> { |
@injectable() | ||
export abstract class GLSPServerLauncher<T = undefined> { | ||
export abstract class GLSPServerLauncher<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide a protected disposeOnStop = new DisposableCollection();
to provide a default stop
behavior that can be filled up by adding disposables to this collection on start
or run
? Maybe this launcher should also implement Disposable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea 👍🏼
|
||
protected createHttpServer(port: number, host?: string): http.Server { | ||
const server = http.createServer((req, res) => { | ||
const body = http.STATUS_CODES[426]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to look up that status code so I think it would be great to either import some package that already provides them or at least create a constant for the one we use const STATUS_UPGRADE_REQUIRED = 426
or something similar.
"${workspaceRoot}/examples/workflow-server/*/lib/**/*.js", | ||
"${workspaceRoot}/packages/**/*/lib/**/*.js" | ||
], | ||
"sourceMapPathOverrides": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you maybe add a short comment on why this is necessary and what the deal is with the webpack
and meteor
URIs?
Co-authored-by: Martin Fleck <mfleck@eclipsesource.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Requires eclipse-glsp/glsp-client#215
Fixes eclipse-glsp/glsp#945