jupyter-server / jupyter_ydoc

Jupyter document structures for collaborative editing using Yjs/pycrdt
https://jupyter-ydoc.readthedocs.io
BSD 3-Clause "New" or "Revised" License
28 stars 17 forks source link

Add model version #139

Closed davidbrochart closed 1 year ago

davidbrochart commented 1 year ago

Closes #138.

davidbrochart commented 1 year ago

IIRC in #138 we were discussing the use of WebSocket subprotocols, but I guess we don't need that now that we can pass the document model version in the DocSessionHandler's reply?

  • What version should we use? I would be in favor of using 1.0.0 and making the next release of the package 1.0.0 as we are going to release JupyterLab 4.0.0 and we have not seen a need for changes here.

Sounds good to me.

  • Should we add the version in all cell types instead of just YBaseCell?

You mean so that e.g. a Markdown cell and a code cell can have different document versions? If yes, I think so.

fcollonval commented 1 year ago

You mean so that e.g. a Markdown cell and a code cell can have different document versions? If yes, I think so.

Yes


Do you have time to push this forward? Or should I do it?

davidbrochart commented 1 year ago

I can do it.

davidbrochart commented 1 year ago

Actually I'm wondering if cells should have a version, since they are not really a document. The Python API only provides a version for documents (notebook, file...).

davidbrochart commented 1 year ago

My idea was to have a version attribute in ISharedDocument, so that all shared documents must set a version. But since they derive from YDocument which implements ISharedDocument, I had to omit version from YDocument, and then there is this error:

src/ydocument.ts(69,7): error TS2416: Property 'changed' in type 'YDocument<T>' is not assignable to the same property in base type 'Omit<ISharedDocument, "version">'.
  Type 'ISignal<this, T>' is not assignable to type 'ISignal<ISharedDocument, DocumentChange>'.
    Type 'this' is not assignable to type 'ISharedDocument'.
      Property 'version' is missing in type 'YDocument<T>' but required in type 'ISharedDocument'.
src/ydocument.ts(76,7): error TS2416: Property 'disposed' in type 'YDocument<T>' is not assignable to the same property in base type 'Omit<ISharedDocument, "version">'.
  Type 'ISignal<this, void>' is not assignable to type 'ISignal<ISharedDocument, void>'.
    Type 'this' is not assignable to type 'ISharedDocument'.

Any idea?

davidbrochart commented 1 year ago

An alternative would be to have an empty version in YDocument (and nothing in ISharedDocument), but that would not enforce shared documents to have a version at compile-time.

fcollonval commented 1 year ago

Actually I'm wondering if cells should have a version, since they are not really a document.
The Python API only provides a version for documents (notebook, file...).

Yes I agree with you it make sense to only apply it for document types.

An alternative would be to have an empty version in YDocument (and nothing in ISharedDocument), but that would not enforce shared documents to have a version at compile-time.

A solution would be to turn that class to an abstract class with version as abstract field: https://www.typescriptlang.org/docs/handbook/2/classes.html#abstract-classes-and-members

davidbrochart commented 1 year ago

A solution would be to turn that class to an abstract class with version as abstract field

Thanks, I tried to turn YDocument into an abstract class but then it cannot have a create method anymore, since an instance of an abstract class cannot be created. I'm not sure how to create a class of the derived class.

fcollonval commented 1 year ago

I would be in favor of dropping it as anyway that class is not meant to be instantiated.

Pinging @hbcarlos for his opinion.

hbcarlos commented 1 year ago

Yes, we can drop it.

davidbrochart commented 1 year ago

This yarn.lock file is driving me crazy. If I commit what I have on my machine, the CI gets a different file. If I match the CI file, it doesn't work :angry:

hbcarlos commented 1 year ago

This yarn.lock file is driving me crazy. If I commit what I have on my machine, the CI gets a different file. If I match the CI file, it doesn't work 😠

I think it was related to the changes in the yarn.lock. This PR should not change that file.