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.

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

GLSP-77: Allow WebSocket connections to reconnect after interrupt #269

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

ndoschek
Copy link
Contributor

@ndoschek ndoschek commented Jul 20, 2023

  • Introduce GLSPWebSocketProvider to allow websocket connections to reconnect after interrupt
  • Make use of ws provider in standalone example

Part of eclipse-glsp/glsp#77


Related to: eclipse-glsp/glsp-server-node#54
Related to: eclipse-glsp/glsp-server#208

Testing:

  • To simulate interrupts, you can use the corresponding testing branch issues/77-testing. This automatically interrupts the example twice with an timeout of 5s in between.

- Introduce GLSPWebSocketProvider to allow websocket connections to reconnect after interrupt
- Make use of ws provider in standalone example

Part of eclipse-glsp/glsp#77
}
})
);
actionDispatcher.dispatch(RequestTypeHintsAction.create());
await actionDispatcher.onceModelInitialized();
actionDispatcher.dispatch(EnableToolPaletteAction.create());

if (isReconnecting) {
const message = `Connection to the ${id} glsp server got closed. Connection was successfully re-established.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Offtopic: Dispatching ServerStatusActions or ServerMessageActions on the client side seems a bit weird.
In general, these actions are just plain status or message notifications and don't have to necessarily come from the server. Maybe we should consider a rename here to a more generic StatusAction and MessageAction or at the very least make it clear in the documentation that this actions can also be dispatched from the client side.
@planger WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

True, I'm in favor of a rename to StatusAction and MessageAction. However, I don't consider this a high prio.

Copy link
Contributor

Choose a reason for hiding this comment

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

Me neither, nevertheless we should probably do it as part of the 2.0 Release. It' an API break after all, even if its just a minor one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll create an issue for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tortmayr
Copy link
Contributor

Thanks Nina. Very promising change. I only had time for a code review for now. Will do some testing on Monday.

@ndoschek ndoschek requested a review from tortmayr July 24, 2023 06:53
@ndoschek ndoschek merged commit 3208dc6 into master Jul 24, 2023
5 checks passed
@ndoschek ndoschek deleted the issues/77 branch July 24, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants