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

✨Collaboration long polling fallback #517

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AntoLC
Copy link
Collaborator

@AntoLC AntoLC commented Dec 18, 2024

Purpose

Some users have their websockets blocked, so they cannot collaborate.
If they are connected with other collaborators at the same time, it will create constant conflict in the document.

Proposal

We will use the long polling as a fallback when the websocket is not able to connect.

Cases we solved:

  • user without ws <--> user with ws
  • user without ws <--> user without ws
  • user without ws first <--> user without ws <--> user with ws
  • manage right editing
  • manage correctly the fallback ( x failures trigger the fallback ?)
  • increase/decrease polling frequency depend the collaborators amount ?
  • helm charts to use ngnix sub auth
  • tests
  • Cursor when not connected with websocket

@AntoLC AntoLC self-assigned this Dec 18, 2024
@AntoLC AntoLC changed the title ✨Collab long polling ✨Collaboration long polling fallback Dec 18, 2024
@AntoLC AntoLC force-pushed the feature/collab-long-polling branch 3 times, most recently from 1360973 to 55238a7 Compare December 23, 2024 16:18
@AntoLC AntoLC changed the base branch from main to refacto/collaboration-process December 23, 2024 16:19
@AntoLC AntoLC mentioned this pull request Dec 23, 2024
4 tasks
Base automatically changed from refacto/collaboration-process to main December 24, 2024 11:29
Create the route '/collaboration/poll/' to interact
with the hocus pocus-server from a http request.
It will be used to poll the server for changes in the
collaboration document.
We add the routes "/collaboration/ws/poll/".
We updated the useCollaborationUrl hook to
return the new route.
We update ngnix to accept OPTIONS requests.
@AntoLC AntoLC force-pushed the feature/collab-long-polling branch 4 times, most recently from 137c5b1 to ff343ca Compare December 24, 2024 15:21
@AntoLC AntoLC marked this pull request as ready for review December 24, 2024 15:21
@AntoLC AntoLC requested a review from YousefED December 24, 2024 15:25
When the websocket cannot connect, after a certain
amount of retry, the system will fallback to long
polling to keep the document sync.
If the websocket is connected, the system will
automatically switch back to websocket and stop
long polling.
If we see that someone is connected to the document,
we increase the polling interval. If no one is connected,
we decrease the polling interval.

For the moment, we can only see users connected
with websockets. A good improvement would be to
see users connected with long polling as well.
@AntoLC AntoLC force-pushed the feature/collab-long-polling branch from ff343ca to d95e892 Compare December 24, 2024 15:32
@virgile-dev
Copy link
Collaborator

Great job @AntoLC !

Copy link
Collaborator

@YousefED YousefED left a comment

Choose a reason for hiding this comment

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

@AntoLC Nice you got this working. If I see it correctly, you created a new endpoint over which you're always syncing the entire Y.Doc to and from the server.

If I'm not mistaken, normally the Y.js sync protocol is more efficient than this and syncs the exact updates required. What's the reason you went for this approach (new endpoint, syncing entire doc) instead of the proxy approach? I think the proxy approach has some potential advantages:

  • We can keep the same sync protocol, but just switch to a different transport (more efficient and awareness would still work)
  • The HocusPocus side can stay the same, our "fix" would be isolated to a separate layer) (less code complexity and smaller chance of bugs or security issues)

I might be missing some advantages of your current approach, but my concern is mainly that it adds more "custom code" that's another surface we need to test, maintain and secure. The proxy approach would isolate / limit this more I think

@@ -13,5 +20,13 @@ export const useCollaborationUrl = (room?: string) => {
? `wss://${window.location.host}/collaboration/ws/`
: '');

return `${base}?room=${room}`;
const wsUrl = `${base}?room=${room}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants