yjs / y-websocket

Websocket Connector for Yjs
https://docs.yjs.dev/ecosystem/connection-provider/y-websocket
MIT License
524 stars 262 forks source link

Add persistence-synced Y.Doc accessor functions #32

Closed canadaduane closed 4 years ago

canadaduane commented 4 years ago

Provides one function for synchronized access to Y.Doc documents in the y-websocket server:

This function can be used downstream by other server components to get or create stored documents, whether they are on-disk only, or in-memory.

This relates to discussion here about allowing a little more access to the ydb-synced documents in general so that custom requirements (like truncating and other unsupported features) can be developed on top of Yjs.

Huly®: YJS-783

canadaduane commented 4 years ago

I need getYDoc as it returns null when the doc doesn't exist (as opposed to findorcreate which never returns null).

Moving to another function outside of the persistence object would be fine. It just made sense to have it there because I thought it should only exist with persistence turned on... But now that I think about it, we could generalize it so it turns the ydoc in memory only if persistence is off.

On Sat, Oct 10, 2020, 9:14 AM Kevin Jahns notifications@github.com wrote:

@dmonad commented on this pull request.

In bin/utils.js https://github.com/yjs/y-websocket/pull/32#discussion_r502800565:

     ldb.storeUpdate(docName, update)

}) },

  • writeState: async (docName, ydoc) => {}
  • writeState: async (docName, ydoc) => {},
  • // Gets a Y.Doc by name, whether in memory or on disk
  • getYDoc: async (docName) => {

I think that findOrCreateDoc should be preferred as it will also work when another persistence layer is defined.p

The idea of the persistence object is that it only has two properties: bindState and writeState.

Should we remove mutualSync and getYDoc or outsource them to a different function?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yjs/y-websocket/pull/32#pullrequestreview-506104562, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAABAJV5NTH5LMPU5VCWOTSKB24FANCNFSM4SJIKUHA .

dmonad commented 4 years ago

Right, agreed. getYDoc is useful but should live outside of the persistence object.

canadaduane commented 4 years ago

Ok, cool. I'm thinking I'll rename getYDoc to findDoc to match findOrCreateDoc, and put it in global scope.

On Sat, Oct 10, 2020 at 9:46 AM Kevin Jahns notifications@github.com wrote:

Right, agreed. getYDoc is useful but should live outside of the persistence object.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yjs/y-websocket/pull/32#issuecomment-706568658, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAABAJGZRPEJAOFICC2TZ3SKB6VFANCNFSM4SJIKUHA .

canadaduane commented 4 years ago

Or maybe getDoc and getOrCreateDoc... hmmm

On Sat, Oct 10, 2020 at 10:27 AM Duane Johnson duane.johnson@gmail.com wrote:

Ok, cool. I'm thinking I'll rename getYDoc to findDoc to match findOrCreateDoc, and put it in global scope.

On Sat, Oct 10, 2020 at 9:46 AM Kevin Jahns notifications@github.com wrote:

Right, agreed. getYDoc is useful but should live outside of the persistence object.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yjs/y-websocket/pull/32#issuecomment-706568658, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAABAJGZRPEJAOFICC2TZ3SKB6VFANCNFSM4SJIKUHA .

dmonad commented 4 years ago

Or maybe getDoc and getOrCreateDoc...

This is more consistent. Thank you!

canadaduane commented 4 years ago

After looking at the code, I realized I was NOT using getYDoc like I thought I was--it, too, was creating a document if it didn't exist. So, I consolidated the two functions. getOrInitDoc is now an internal function only, while getOrCreateDoc (was getYDoc) is exported.

canadaduane commented 4 years ago

Note: I'm still testing, will get back in a moment with test results.

canadaduane commented 4 years ago

Ok, I tested this and it looks good. The only thing that came up was an old dependency on yjs was causing a mismatch in some yjs function calls, but bumping to latest resolved it.

dmonad commented 4 years ago

Hey @canadaduane I simplified this PR and I think that most use-cases are now covered in #37