Add `.clone()` method to Live structures #1245

nvie · 2023-10-20T08:37:12Z

Fixes https://github.com/liveblocks/liveblocks.io/issues/1594, #560, and #1000.

This adds a .clone() method to LiveMap, LiveList, and LiveObject, to make a deep clone of those values that are not attached to the pool, and thus are suitable for injection into another document, or in a different part of the Storage tree. Previously, users have been using this community-provided code snippet.

This method can be useful to implement features like "duplicate selected layers" in a drawing app, for example.

Usage:

const shapes = root.get("shapes");
const shape1 = shapes.get("shape1");
shapes.set("shape2", shape1.clone());

TODO

~~Only clone when needed, and issue a warning when we do~~ Update: decided not to
Unit test this
Check if this can be a good default solution to avoid "Cannot set parent: node already has a parent" #1000
Documentation

packages/liveblocks-core/src/crdts/LiveList.ts

packages/liveblocks-core/src/crdts/LiveMap.ts

nvie · 2023-10-20T08:40:01Z

packages/liveblocks-core/src/crdts/__tests__/_arbitraries.ts

+export const { liveList, liveMap, liveObject, liveStructure, lson } = fc.letrec(
+  (tie) => ({
+    lson: fc.oneof(json, tie("liveStructure")).map((x) => x as Lson),
+    liveStructure: fc
+      .oneof(tie("liveList"), tie("liveMap"), tie("liveObject"))
+      .map((x) => x as LiveStructure),
+    liveList: fc.array(tie("lson")).map((x) => new LiveList(x as Lson[])),
+    liveMap: fc
+      .array(fc.tuple(key, tie("lson")))
+      .map((pairs) => new LiveMap(pairs as [string, Lson][])),
+    liveObject: fc
+      .array(fc.tuple(key, tie("lson")))
+      .map(
+        (pairs) => new LiveObject(Object.fromEntries(pairs as [string, Lson][]))
+      ),
+  })
+);


These fast-check arbitraries will generate arbitrarily deep recursive LSON trees.

nvie · 2023-10-20T08:45:04Z

packages/liveblocks-core/src/crdts/__tests__/cloning.test.ts

+  it("large/deep cloning", () =>
+    fc.assert(
+      fc.asyncProperty(
+        liveStructure,
+
+        async (data) => {
+          const { root } = await prepareStorageUpdateTest([
+            createSerializedObject("0:0", {}),
+          ]);
+
+          // Clone "a" to "b"
+          root.set("a", data);
+          root.set("b", data.clone());
+
+          const imm = root.toImmutable();
+          expect(imm.a).toEqual(imm.b);
+        }
+      )
+    ));


Here's the simplest test explained:

First, we use the liveStructure Arbitrary (see above) to generate randomized LiveMap/LiveList/LiveObject structures. (Some deep, some shallow, some simple, some complex, etc.)

Then, we inject it into the root, under key "a". Next, we clone the value and inject it under key "b". Note that inserting the same value into the tree in different places is illegal and would lead to an error, e.g.

root.set("a", data); root.set("b", data); // ❌ Would error!

But not when you clone it—then it's fine:

root.set("a", data); root.set("b", data.clone()); // ✅ Fine!

Finally, we check that the cloned tree will produce the same immutable value for keys "a" and "b".

nvie · 2023-10-20T11:12:04Z

packages/liveblocks-core/src/room.ts

@@ -1341,7 +1342,7 @@ export function createRoom<
    const stackSizeBefore = context.undoStack.length;
    for (const key in context.initialStorage) {
      if (context.root.get(key) === undefined) {
-        context.root.set(key, context.initialStorage[key]);
+        context.root.set(key, cloneLson(context.initialStorage[key]));


@GuillaumeSalles What do you think of doing this when initializing initialStorage? This would avoid the error from #1000 from ever showing up, even if people set it up using a global instance accidentally. I think this is pretty nice, but not sure if maybe you see a caveat with this?

We just chatted about this, and decided to change this slightly to the following behavior:

We do a quick check to see if context.initialStorage[key] is a Live structure

If so, we check to see if it has a parent node set (i.e. it's part of a tree already)

If it has a parent, we .clone() it, and issue a warning that this can be indicative of a bug in the way the app is configured, and that the user should always use fresh/new instances

If not, great, just use it

If not, it's a JSON value, so no need to clone anything

Will implement this next week.

Now that I started implementing this, I'm having second thoughts about this solution, because doing the optional cloning leads to situations that are maybe even more confusing, potentially.

Here's such a footgun:

First, empty/new room A is entered with initialStorage={initialStorage}

Keys are missing, so the global initialStorage values are assigned, no cloning

User now just uses the room, mutating things as they go, populating the document with whatever content. Since no clone wat taken, this will also mutate the global initialStorage instance (unintentionally)

Now, the user joins empty/new room B with initialStorage={initialStorage}

Keys are missing, but since global initialStorage values are detected to already have a parent, a clone is taken, but… the clone will now contain all the mutations made in room A¹!

This makes me conclude that the optional cloning tactic is maybe the worst solution even.

Therefore, I think that always cloning (i.e. what this PR implemented) is still be best pragmatic solution that leads to the fewest surprises — and reflects best how the prop is intended. We can document that whatever you pass as initialStorage will be considered a "template".

cc @GuillaumeSalles

Footnotes

Not only is this buggy, it also is inconsistent if you do the same thing again: mutate room B's contents, join new/empty room C. Now room C is another copy of room A, not B! ↩

nvie · 2023-10-20T11:12:46Z

packages/liveblocks-core/src/__tests__/room.test.ts

+  test("missing storage keys are properly initialized using initialStorage, even when they are already part of the storage tree", async () => {
+    const initialPresence = {};
+    const initialStorage = {
+      list: new LiveList([13, 42]),
+      map: new LiveMap([["a", 1]]),
+      obj: new LiveObject({ x: 0 }),
+    };
+
+    // Create the same room instance twice (using the same, global,
+    // initialStorage instance)
+    for (let i = 0; i < 2; i++) {
+      const { room, wss } = createTestableRoom(
+        initialPresence,
+        undefined,
+        undefined,
+        undefined,
+        initialStorage
+      );
+      wss.onConnection((conn) => {
+        conn.server.send(
+          serverMessage({
+            type: ServerMsgCode.INITIAL_STORAGE_STATE,
+            items: [["root", { type: CrdtType.OBJECT, data: {} }]],
+            //                                              ^^
+            //                                   NOTE: Storage is initially
+            //                                   empty, so initialStorage keys
+            //                                   will get added
+          })
+        );
+      });
+      room.connect();
+      try {
+        await room.getStorage();
+      } finally {
+        room.destroy();
+      }
+    }
+  });


This test fails on main, but passes with this fix applied.

…initialStorage value twice works fine

This avoids the "Cannot set parent: node already has a parent" error.

jrowny

looks good!

… the second room when initialized

nvie requested review from jrowny, GuillaumeSalles and CTNicholas October 20, 2023 08:37

nvie self-assigned this Oct 20, 2023

nvie commented Oct 20, 2023

View reviewed changes

packages/liveblocks-core/src/crdts/LiveList.ts Show resolved Hide resolved

nvie commented Oct 20, 2023

View reviewed changes

packages/liveblocks-core/src/crdts/LiveMap.ts Show resolved Hide resolved

nvie commented Oct 20, 2023

View reviewed changes

nvie mentioned this pull request Oct 20, 2023

Release 1.5 #1244

Merged

13 tasks

nvie force-pushed the clone-api branch from fa020be to 292f07f Compare October 20, 2023 11:09

vercel bot deployed to Preview – nextjs-starter-kit October 20, 2023 11:12 View deployment

nvie commented Oct 20, 2023

View reviewed changes

nvie linked an issue Oct 20, 2023 that may be closed by this pull request

"Cannot set parent: node already has a parent" #1000

Open

3 tasks

nvie added this to the 1.5 milestone Oct 20, 2023

vercel bot deployed to Preview – nextjs-starter-kit October 20, 2023 11:30 View deployment

nvie added 8 commits October 20, 2023 14:31

Style cleanups

1eb3f5a

Add .clone() method on all CRDTs

1dde223

Upgrade fast-check

02098fc

Add unit tests for cloning behavior

1280d25

Add cloneLson helper (for when the value is Lson, instead of a LiveNode)

5f71c3a

Add test to ensure creating two room instances using the same global …

cb17c92

…initialStorage value twice works fine

Always clone initial storage keys

31f2068

This avoids the "Cannot set parent: node already has a parent" error.

Write the docs, and update changelog

1d5bca3

nvie force-pushed the clone-api branch from 6022609 to 1d5bca3 Compare October 20, 2023 12:31

Lint

e40dcda

vercel bot deployed to Preview – nextjs-starter-kit October 20, 2023 13:05 View deployment

jrowny approved these changes Oct 20, 2023

View reviewed changes

Extend test to mutate room and ensure those mutations don't end up in…

3f33619

… the second room when initialized

nvie merged commit 90cb806 into release-1.5 Oct 23, 2023
36 of 53 checks passed

nvie deleted the clone-api branch October 23, 2023 08:18

vercel bot deployed to Preview – nextjs-starter-kit October 23, 2023 08:18 View deployment

vercel bot had a problem deploying to Preview – examples-nextjs-comments-video October 23, 2023 08:19 Failure

vercel bot had a problem deploying to Preview – examples-nextjs-comments-tiptap October 23, 2023 08:19 Failure

vercel bot had a problem deploying to Preview – examples-nextjs-comments-overlay October 23, 2023 08:19 Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `.clone()` method to Live structures #1245

Add `.clone()` method to Live structures #1245

nvie commented Oct 20, 2023 •

edited

nvie Oct 20, 2023

nvie Oct 20, 2023

nvie Oct 20, 2023

nvie Oct 20, 2023

nvie Oct 20, 2023

nvie Oct 23, 2023 •

edited

nvie Oct 20, 2023

jrowny left a comment

Add .clone() method to Live structures #1245

Add .clone() method to Live structures #1245

Conversation

nvie commented Oct 20, 2023 • edited

TODO

nvie Oct 20, 2023

Choose a reason for hiding this comment

nvie Oct 20, 2023

Choose a reason for hiding this comment

nvie Oct 20, 2023

Choose a reason for hiding this comment

nvie Oct 20, 2023

Choose a reason for hiding this comment

nvie Oct 20, 2023

Choose a reason for hiding this comment

nvie Oct 23, 2023 • edited

Choose a reason for hiding this comment

Footnotes

nvie Oct 20, 2023

Choose a reason for hiding this comment

jrowny left a comment

Choose a reason for hiding this comment

Add `.clone()` method to Live structures #1245

Add `.clone()` method to Live structures #1245

nvie commented Oct 20, 2023 •

edited

nvie Oct 23, 2023 •

edited