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

Save document through Yjs websocket #624

Closed
wants to merge 2 commits into from

Conversation

davidbrochart
Copy link
Contributor

@davidbrochart davidbrochart commented Dec 1, 2021

Description

The save command still goes through the HTTP endpoint, but the document source is sent to the Yjs websocket.

Why do we need this?

When using RTC, the document doesn't live in a particular front-end, or rather, it lives in all front-ends. All front-ends' versions of the document are kept in sync with Yjs. The document updates coming from each front-end go to the RTC server (the Yjs websocket), which forwards them to every other front-end. Thus, the RTC server is aware of all the changes that have been made to the document. It could very well compute the document content, but today y-py is not able to do so (it will soon). The document's source should come from the RTC server, instead of a particular front-end (which has to compute the document content and send it over the network, while it is already there in the back-end).
Because we are not quite ready to use y-py in the RTC server, we propose as a first step to have the front-end compute the document and send it to the RTC server (websocket) rather than to the HTTP endpoint, when saving the document. The save "command" still goes to the HTTP endpoint, but the document's content is retrieved from the RTC server. We will completely remove the sending of the document by the front-end when the RTC server is able to compute the document using y-py.

cc @dmonad

See:

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@4d2f85a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #624   +/-   ##
=======================================
  Coverage        ?   77.67%           
=======================================
  Files           ?      110           
  Lines           ?    10483           
  Branches        ?     1423           
=======================================
  Hits            ?     8143           
  Misses          ?     1933           
  Partials        ?      407           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d2f85a...7e9de4b. Read the comment docs.

@Zsailer
Copy link
Member

Zsailer commented Dec 6, 2021

We discussed this a bit in the Jupyter Server meeting, but an alternative approach—that wouldn't require these conditional changes in Jupyter Server that are unique to JupyterLab today—would be to have a custom Y.js-based ContentsManager in JupyterLab Server that configures the contents_manager_class if y-py is found at runtime.

In the future, we can consider bringing the yjs websocket channel and this yjs ContentsManager into Jupyter Server if it can be easily generalized for other Jupyter applications. For now, I think it makes sense to keep this work closer to JupyterLab as the test bed.

@davidbrochart
Copy link
Contributor Author

This PR is now ready for review.
cc @dmonad

@davidbrochart
Copy link
Contributor Author

Closing as it's now implemented using a pre_save_hook in jupyterlab/jupyterlab#11599.

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