Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Creates document session #108
Creates document session #108
Changes from all commits
f69a150
0cea8f0
626339f
7d9e5bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't think we should grow the API surface.
I think we should use the already existing
PUT /api/yjs/roomid/{path}
endpoint. It could return a value that is a combination of the file ID, the document type and format, and the session ID. That way, the frontend doesn't have to ask for a session ID, all it asks for is a room ID. When it tries to connect to the WebSocket with a room ID that doesn't exist (because the server session has changed), the room cannot be found and the frontend asks for the room ID again, discarding the shared document that it may have.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'm afraid I have to disagree. Returning a string with multiple attributes in it is not the proper solution.
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.
By combination, I don't mean necessarily concatenating multiple attributes, it can be a UUID and the server keeps a mapping of this UUID to the attributes.
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.
The trouble with the opaque UUID is that on the frontend you will not be able to provide good error message to the user and follow-up action. In particular if the reason the websocket cannot be opened is the session id, we must tell the user to not use the current browser tab.
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.
It is easy for the frontend to know that the session has changed: it fails to connect with its room ID. It can now ask for the room ID again, if that fails it's because the document doesn't exist, otherwise it's because the server has restarted, and in that case it can tell the user to not use the tab.
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.
Implicit deduction is never a good path for code evolution, maintenance and understanding.