valtiojs / valtio-yjs

valtio-yjs makes yjs state easy
MIT License
270 stars 15 forks source link

binding should batch transactions #22

Closed BrianHung closed 1 year ago

BrianHung commented 2 years ago

Right now, when binding an object that has n values, it creates >= n separate Yjs transactions since transact is called independently. Expected behavior is that binding an object creates only one transaction.

From Yjs,

Every change on the shared document happens in a transaction. Observer calls and the update event are called after each transaction. You should bundle changes into a single transaction to reduce the amount of event calls. I.e. doc.transact(() => { yarray.insert(..); ymap.set(..) }) triggers a single change event.

Need to somehow "unroll" bindProxyAndYMap and bindProxyAndYArray such that transact is not nested and called multiple times per binding.

Object.entries(p).forEach(([k, pv]) => {
  transact(...)
}

to

transact(
  Object.entries(p).forEach(([k, pv]) => { ... }
)

Would require making these two functions iterative instead of recursive (?).

dai-shi commented 2 years ago

yeah, this sound tough. it might require big refactor.

BrianHung commented 2 years ago

Two things having looked more into this:

  1. Since each field update from valtio proxy to Yjs object is independently transacted (even if they're batched in valtio), it breaks the expected behavior when using valtio-yjs with Y.UndoManager: one yjs undo should undo one batched valtio update instead of undoing single field updates.
  2. The initialization step should probably have a different transaction origin than from subscription updates. This is so you can't undo the initialization.