Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GLSP-975 Ensure generic GLSP Server implementation #44

Merged
merged 5 commits into from
May 3, 2023
Merged

GLSP-975 Ensure generic GLSP Server implementation #44

merged 5 commits into from
May 3, 2023

Conversation

tortmayr
Copy link
Contributor

  • Introduce a common subclass for jsonrpc based ServerLaunchers

  • Move out jsonrpc implementation details from GLSPServer implementation into the JsonRpcGLSPServerLauncher. This ensures that the Server implementation itself is independent from the chosen communication channel and can reused for other use cases like direct invocation in a node backend or orthogonal communication channels like REST or gRPC.

  • Update to latest @eclipse-glsp/protocol version and consume shared server interface types from there

  • Ensure that the @eclipse-glsp-examples/workflow-server package is properly indexed

  • Add entry points for browser and node

  • Move rpc-server initialization out of index files into app files.

Also: Update example applications to ensure that promise rejections/errors are properly catched.

Part of eclipse-glsp/glsp#975

Requires eclipse-glsp/glsp-client#245

Contributed on behalf of STMicroelectronics

tortmayr added a commit to eclipse-glsp/glsp-theia-integration that referenced this pull request Apr 24, 2023
@tortmayr
GLSP-975 Support for servers directly in the Theia backend
a533eef 
-Introduce a new `GLSPNodeServerContribution` that can be used in scenarios where the GLSPServer should be running directly in the backend
- Update to latest protocol version to make use of the newly exposed server types.
- Update lauch configs,tasks and scripts to also cover the new integrated use case.
- Add a flag to the workflow-backend-module to be able to switch between the default and integrated usecase
Update linking script to also properly link the node-server packages
- Move `ChannelConnection` into common package to also resuse it in the backend

Part of eclipse-glsp/glsp#975

Requires eclipse-glsp/glsp-client#245
Requires eclipse-glsp/glsp-server-node#44

Contributed on behalf of STMicroelectronics
@tortmayr tortmayr mentioned this pull request Apr 24, 2023
Merged
@tortmayr tortmayr requested a review from sgraband April 24, 2023 14:36
sgraband
sgraband reviewed Apr 25, 2023
View reviewed changes
Copy link
Contributor

@sgraband sgraband left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes! They look good and work great! I just have a few questions/comments inline below. Could you take a look at them?

packages/server/src/common/utils/console-logger.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should replace packages/server/src/browser/di/console-logger?
The old file however was not removed as far as i can see. Could you do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Yes exactly, the intention was to move the generic ConsoleLogger from the browser into the common package. I will remove the redundant file.

packages/server/src/common/session/client-session-listener.ts Outdated
@@ -13,6 +13,7 @@
*
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but this just introduces an empty line, which is not required by the linter afaik.

examples/workflow-server/browser.d.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be checked in?
If yes, could we maybe add a comment for why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes these files are needed.
This is a pattern used for packages that provide both a browser and node entry point . We already do the same in the server package.

It allows to import either from @eclipse-glsp-examples/workflow-server/node or @eclipse-glsp/examples/workflow-server/browser

Since this is a common pattern also used by other projects (e.g.vscode-language-server (https://github.com/microsoft/vscode-languageserver-node/tree/main/server) I'm not sure if this really requires a comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thank you for the explanation. I was simply confused by the location of the files, but i don't think we need a comment then.

examples/workflow-server/browser.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be checked in?
If yes, could we maybe add a comment for why this is needed?

examples/workflow-server/node.d.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be checked in?
If yes, could we maybe add a comment for why this is needed?

examples/workflow-server/node.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be checked in?
If yes, could we maybe add a comment for why this is needed?

tortmayr added 3 commits May 3, 2023 15:07
@tortmayr
GLSP-975 Ensure generic GLSP Server implementation
2e57c4e 
- Introduce a common subclass for jsonrpc based ServerLaunchers
- Move out jsonrpc implementation details from `GLSPServer` implementation into the `JsonRpcGLSPServerLauncher`.
 This ensures that the Server implementation itself is independent from the chosen communication channel and can reused for other use cases like direct invocation in a node backend or orthogonal communication channels like REST or gRPC.
- Update to latest @eclipse-glsp/protocol version and consume shared server interface types from there

- Ensure that the `@eclipse-glsp-examples/workflow-server` package is properly indexed
- Add entry points for browser and node 
- Move rpc-server initialization out of index files into app files. 


Also: Update example applications to ensure that promise rejections/errors are properly catched.

Part of eclipse-glsp/glsp#975

Requires eclipse-glsp/glsp-client#245

Contributed on behalf of STMicroelectronics
@tortmayr
Address review feedback
9eedaa9
@tortmayr
Update yarn.lock
b8c0d26
@tortmayr tortmayr force-pushed the combined-backend branch from a962a94 to b8c0d26 Compare May 3, 2023 13:11
@tortmayr tortmayr merged commit 3bba361 into main May 3, 2023
@tortmayr tortmayr deleted the combined-backend branch May 3, 2023 15:06
tortmayr added a commit to eclipse-glsp/glsp-theia-integration that referenced this pull request May 5, 2023
@tortmayr
GLSP-975 Support for servers directly in the Theia backend
8e898ad 
-Introduce a new `GLSPNodeServerContribution` that can be used in scenarios where the GLSPServer should be running directly in the backend
- Update to latest protocol version to make use of the newly exposed server types.
- Update lauch configs,tasks and scripts to also cover the new integrated use case.
- Add a flag to the workflow-backend-module to be able to switch between the default and integrated usecase
Update linking script to also properly link the node-server packages
- Move `ChannelConnection` into common package to also resuse it in the backend

Part of eclipse-glsp/glsp#975

Requires eclipse-glsp/glsp-client#245
Requires eclipse-glsp/glsp-server-node#44

Contributed on behalf of STMicroelectronics
tortmayr added a commit to eclipse-glsp/glsp-theia-integration that referenced this pull request May 5, 2023
@tortmayr
GLSP-975 Support for servers directly in the Theia backend (#154)
a172d67 
* GLSP-975 Support for servers directly in the Theia backend

-Introduce a new `GLSPNodeServerContribution` that can be used in scenarios where the GLSPServer should be running directly in the backend
- Update to latest protocol version to make use of the newly exposed server types.
- Update lauch configs,tasks and scripts to also cover the new integrated use case.
- Add a flag to the workflow-backend-module to be able to switch between the default and integrated usecase
Update linking script to also properly link the node-server packages
- Move `ChannelConnection` into common package to also resuse it in the backend

Part of eclipse-glsp/glsp#975

Requires eclipse-glsp/glsp-client#245
Requires eclipse-glsp/glsp-server-node#44

Contributed on behalf of STMicroelectronics
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgraband sgraband sgraband left review comments

Assignees
No one assigned
Labels
changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tortmayr @sgraband