TypeError is thrown when inserting consecutive fields into arrays

joebobmiles commented 3 years ago

Noticed in the RPG code that we're getting a TypeError from Yjs:

    TypeError: Cannot read property 'forEach' of null

      26 |       addActor:
      27 |         (actor: Actor) =>
    > 28 |           set((state) => ({ actors: [ ...state.actors, actor ] })),
         |           ^
      29 |       removeActor:
      30 |         (actorId: string) =>
      31 |           set(

      at YMap._integrate (../node_modules/yjs/src/types/YMap.js:77:60)
      at ContentType.integrate (../node_modules/yjs/src/structs/ContentType.js:100:15)
      at Item.integrate (../node_modules/yjs/src/structs/Item.js:506:20)
      at ../node_modules/yjs/src/types/AbstractType.js:672:20
          at Array.forEach (<anonymous>)
      at typeListInsertGenericsAfter (../node_modules/yjs/src/types/AbstractType.js:648:11)
      at typeListInsertGenerics (../node_modules/yjs/src/types/AbstractType.js:701:12)
      at ../node_modules/yjs/src/types/YArray.js:134:9
      at transact (../node_modules/yjs/src/utils/Transaction.js:391:5)
      at YArray.insert (../node_modules/yjs/src/types/YArray.js:133:7)
      at ../node_modules/zustand-middleware-yjs/dist/yjs.cjs:253:40
          at Array.forEach (<anonymous>)
      at patchSharedType (../node_modules/zustand-middleware-yjs/dist/yjs.cjs:228:13)
      at ../node_modules/zustand-middleware-yjs/dist/yjs.cjs:270:21
          at Array.forEach (<anonymous>)
      at patchSharedType (../node_modules/zustand-middleware-yjs/dist/yjs.cjs:228:13)
      at ../node_modules/zustand-middleware-yjs/dist/yjs.cjs:351:13
      at Object.addActor (store/shared.ts:28:11)
      at components/control-panel/ControlPanel.spec.tsx:104:33

I've replicated the bug with the following test:

type Store =
{
  users: { name: string, status: "online" | "offline" }[],
  addUser: (name: string, status: "online" | "offline") => void,
};

const doc = new Y.Doc();

const api =
  create<Store>(yjs(
    doc,
    "hello",
    (set) =>
      ({
        "users": [],
        "addUser": (name, status) =>
          set((state) =>
            ({
              "users": [
                ...state.users,
                {
                  "name": name,
                  "status": status,
                }
              ],
            })),
      })
  ));

expect(api.getState().users).toEqual([]);

expect(() =>
{
  api.getState().addUser("alice", "offline");
  api.getState().addUser("bob", "offline");
}).not.toThrow();

I'm unsure if this is a bug in Yjs or a bug in the middleware.

joebobmiles commented 3 years ago

Probing the extents of this bug, seems that it only happens when inserting data that is translated to a shared type. For instance, this test, which inserts consecutive scalar values, passes:

type Store =
{
  numbers: number[],
  addNumber: (n: number) => void,
};

const doc = new Y.Doc();

const api =
  create<Store>(yjs(
    doc,
    "hello",
    (set) =>
      ({
        "numbers": [],
        "addNumber": (n) =>
          set((state) =>
            ({
              "numbers": [
                ...state.numbers,
                n
              ],
            })),
      })
  ));

expect(api.getState().numbers).toEqual([]);

expect(() =>
{
  api.getState().addNumber(0);
  api.getState().addNumber(1);
}).not.toThrow();

joebobmiles commented 3 years ago

Ok, so there seems to be two bugs, both only occur when performing consecutive inserts into Yjs Arrays. The first happens when inserting maps, which results in a TypeError: Cannot read property 'forEach' of null. The second happens when inserting arrays, which results in a TypeError: Cannot read property 'length' of null. I'm still unsure if these are bugs in Yjs, or something is wrong with the middleware.

joebobmiles commented 3 years ago

I have found where the bug is located in the middleware. It happens when a nested shared type does not change, but is being re-inserted into the parent shared type. In essence, Yjs annotates all shared types with the data that is to be inserted/modified during a transaction. However, if a type is to be inserted, but has not changed since the last transaction, this annotation is null, resulting in the above type errors.

I feel like this is an oversight of Yjs, but a workaround would just be being smart about how we transact on YArrays when processing changes.

A proposed change would be, instead of wholly deleting the contents of the array and re-inserting, would be targetted deletion and insertion to only insert data that has changed.

joebobmiles / zustand-middleware-yjs

TypeError is thrown when inserting consecutive fields into arrays #20