Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LiveList.set #147

Merged
merged 14 commits into from
Apr 22, 2022
Merged

LiveList.set #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c71bec2
List.set basic implementation
GuillaumeSalles Apr 14, 2022
5b3d2e6
List.set nested object
GuillaumeSalles Apr 14, 2022
ff7527d
Refac _attachChild
GuillaumeSalles Apr 14, 2022
f2693e9
Merge branch 'main' into list-set
GuillaumeSalles Apr 14, 2022
5c2ce41
e2e fuzzy tests for LiveList.set
GuillaumeSalles Apr 14, 2022
70a0b22
Make sure List.set next sandbox does not crash is list is empty
GuillaumeSalles Apr 14, 2022
12543c1
LiveList.set unit and integrations tests
GuillaumeSalles Apr 15, 2022
99ad3c8
Merge branch 'main' into list-set
GuillaumeSalles Apr 20, 2022
4202337
Add internal documentation
GuillaumeSalles Apr 20, 2022
92f021c
Merge branch 'list-set' of github.com:liveblocks/liveblocks into list…
GuillaumeSalles Apr 20, 2022
e4ad97b
Fix type after merge
GuillaumeSalles Apr 20, 2022
b69aa07
Fix types after merge
GuillaumeSalles Apr 20, 2022
df40589
Add LiveList.set tests for invalid value
GuillaumeSalles Apr 22, 2022
50de49e
Merge branch 'main' into list-set
GuillaumeSalles Apr 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions e2e/next-sandbox/pages/storage/list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,21 @@ function Sandbox() {
Move
</button>

<button
id="set"
onClick={() => {
if (list.length === 0) {
return;
}

const index = generateRandomNumber(list.length - 1);
list.set(index, me.connectionId + ":" + item);
item = String.fromCharCode(item.charCodeAt(0) + 1);
}}
>
Set
</button>

<button
id="delete"
onClick={() => {
Expand Down
27 changes: 26 additions & 1 deletion e2e/next-sandbox/test/storage/list.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
} from "../utils";

function pickRandomAction() {
return pickRandomItem(["#push", "#delete", "#move"]);
return pickRandomItem(["#push", "#delete", "#move", "#set"]);
}

const TEST_URL = "http://localhost:3007/storage/list";
Expand Down Expand Up @@ -92,6 +92,31 @@ test.describe("Storage - LiveList", () => {
await assertContainText(pages, "0");
});

test("set conflicts", async () => {
await pages[0].click("#clear");
await assertContainText(pages, "0");

for (let i = 0; i < 10; i++) {
// no await to create randomness
pages[0].click("#push");
pages[1].click("#push");
await delay(50);
}

for (let i = 0; i < 10; i++) {
// no await to create randomness
pages[0].click("#set");
pages[1].click("#set");
await delay(50);
}

await assertContainText(pages, "20");
await waitForContentToBeEquals(pages);

await pages[0].click("#clear");
await assertContainText(pages, "0");
});

test("fuzzy with full undo/redo", async () => {
await pages[0].click("#clear");
await assertContainText(pages, "0");
Expand Down
22 changes: 13 additions & 9 deletions packages/liveblocks-client/src/AbstractCrdt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Op, OpType, SerializedCrdt } from "./live";
import { CreateOp, Op, OpType, SerializedCrdt } from "./live";
import { StorageUpdate } from "./types";

export type ApplyResult =
Expand All @@ -12,6 +12,11 @@ export interface Doc {
getItem: (id: string) => AbstractCrdt | undefined;
addItem: (id: string, item: AbstractCrdt) => void;
deleteItem: (id: string) => void;
/**
* - Send ops to WebSocket servers
* - Add reverse operations to the undo/redo stack
* - Send updates to room subscribers
*/
dispatch: (
ops: Op[],
reverseOps: Op[],
Expand Down Expand Up @@ -103,13 +108,7 @@ export abstract class AbstractCrdt {
/**
* @internal
*/
abstract _attachChild(
id: string,
key: string,
crdt: AbstractCrdt,
opId: string,
isLocal: boolean
): ApplyResult;
abstract _attachChild(op: CreateOp, isLocal: boolean): ApplyResult;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR directly, but what does isLocal signify?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isLocal = true means the operation has been created locally by the current client (via undo, redo or re-applying offline ops).
isLocal = false means the operation has been created by another client and received from the server.


/**
* @internal
Expand All @@ -130,7 +129,12 @@ export abstract class AbstractCrdt {
/**
* @internal
*/
abstract _serialize(parentId: string, parentKey: string, doc?: Doc): Op[];
abstract _serialize(
parentId: string,
parentKey: string,
doc?: Doc,
intent?: "set"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably still just my lack of understanding, but I don't have a good intuition yet for why you would have to provide the intent during serialization (more or less as a "mode").

So why do you have to specify the intent at the moment you invoke “serialize”? Does the default "create" op mean to "create and insert", and "create with set intent" mean to "create and replace"? If so, could there be long term maintainability benefits to using different named ops for these actions instead of adding a "mode" to the existing create op?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great question :)

As discussed yesterday, storage operations would be easier to reason about if they were always associated with a specific LiveStructure type.

For example, CreateObjectOp is sent when calling the following methods:

  • liveObject.set("key", new LiveObject());
  • liveList.insert(0, new LiveObject());
  • liveList.set(0, new LiveObject());
  • liveMap.set("key", new LiveObject());

We could have different operations for each of these methods. LiveObjectSetOp, LiveListInsertOp, etc. It's more intuitive at first glance. However, the payload of all these operations becomes more "generic"; LiveListInsertOp could insert a LiveObject, a LiveMap, or any other data structure that we might support in the future. This also introduces complexity for nested data structures (e.g. liveList.insert(0, new LiveList([new LiveObject({ a : 0 })]));) that does not exist with the existing operation structure.

The more I think about it, the more I think we should go in that direction despite the issues above. Especially after introducing the "set intent". But since these operations are internal messages, I would wait to have more operations and data structures (LiveText, LiveBlob, etc) to refactor to a more unified solution. I just want more examples to be sure that this refactoring is really the way to go. So I share your feeling :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, perfect! Then I think we're on the same page here. We can use this API for now, and refactor this later when we have more data points and a better understanding of how to structure this 👍 Thanks for the context.

): Op[];

/**
* @internal
Expand Down
214 changes: 214 additions & 0 deletions packages/liveblocks-client/src/LiveList.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,221 @@ describe("LiveList", () => {
});
});

describe("set", () => {
it("set register on detached list", () => {
const list = new LiveList<string>(["A", "B", "C"]);
list.set(0, "D");
expect(list.toArray()).toEqual(["D", "B", "C"]);
});

it("set at invalid position should throw", () => {
const list = new LiveList<string>(["A", "B", "C"]);
expect(() => list.set(-1, "D")).toThrowError(
`Cannot set list item at index "-1". index should be between 0 and 2`
);
expect(() => list.set(3, "D")).toThrowError(
`Cannot set list item at index "3". index should be between 0 and 2`
);
});

it("set register", async () => {
const { storage, assert, assertUndoRedo } = await prepareStorageTest<{
items: LiveList<string>;
}>(
[
createSerializedObject("0:0", {}),
createSerializedList("0:1", "0:0", "items"),
createSerializedRegister("0:2", "0:1", FIRST_POSITION, "A"),
createSerializedRegister("0:3", "0:1", SECOND_POSITION, "B"),
createSerializedRegister("0:4", "0:1", THIRD_POSITION, "C"),
],
1
);

const root = storage.root;
const items = root.toObject().items;

assert({ items: ["A", "B", "C"] });

items.set(0, "D");
assert({ items: ["D", "B", "C"] });

items.set(1, "E");
assert({ items: ["D", "E", "C"] });

assertUndoRedo();
});

it("set nested object", async () => {
const { storage, assert, assertUndoRedo } = await prepareStorageTest<{
items: LiveList<LiveObject<{ a: number }>>;
}>(
[
createSerializedObject("0:0", {}),
createSerializedList("0:1", "0:0", "items"),
createSerializedObject("0:2", { a: 1 }, "0:1", FIRST_POSITION),
],
1
);

const root = storage.root;
const items = root.toObject().items;

assert({ items: [{ a: 1 }] });

items.set(0, new LiveObject({ a: 2 }));
assert({ items: [{ a: 2 }] });

assertUndoRedo();
});
});

describe("apply CreateRegister", () => {
it(`with intent "set" should replace existing item`, async () => {
const { assert, applyRemoteOperations, subscribe } =
await prepareIsolatedStorageTest<{ items: LiveList<string> }>(
[
createSerializedObject("root", {}),
createSerializedList("0:0", "root", "items"),
createSerializedRegister("0:1", "0:0", FIRST_POSITION, "A"),
],
1
);

assert({
items: ["A"],
});

applyRemoteOperations([
{
type: OpType.CreateRegister,
id: "0:2",
parentId: "0:0",
parentKey: FIRST_POSITION,
data: "B",
intent: "set",
},
]);

assert({
items: ["B"],
});
});

it(`with intent "set" should notify with a "set" update`, async () => {
const { root, applyRemoteOperations, subscribe } =
await prepareIsolatedStorageTest<{ items: LiveList<string> }>(
[
createSerializedObject("root", {}),
createSerializedList("0:0", "root", "items"),
createSerializedRegister("0:1", "0:0", FIRST_POSITION, "A"),
],
1
);

const items = root.get("items");

const callback = jest.fn();

subscribe(items, callback, { isDeep: true });

applyRemoteOperations([
{
type: OpType.CreateRegister,
id: "0:2",
parentId: "0:0",
parentKey: FIRST_POSITION,
data: "B",
intent: "set",
},
]);

expect(callback).toHaveBeenCalledWith([
{
node: items,
type: "LiveList",
updates: [{ type: "set", index: 0, item: "B" }],
},
]);
});

it(`with intent "set" should insert item if conflict with a delete operation`, async () => {
const { root, assert, applyRemoteOperations, subscribe } =
await prepareIsolatedStorageTest<{ items: LiveList<string> }>(
[
createSerializedObject("root", {}),
createSerializedList("0:0", "root", "items"),
createSerializedRegister("0:1", "0:0", FIRST_POSITION, "A"),
],
1
);

const items = root.get("items");

assert({
items: ["A"],
});

items.delete(0);

assert({
items: [],
});

applyRemoteOperations([
{
type: OpType.CreateRegister,
id: "0:2",
parentId: "0:0",
parentKey: FIRST_POSITION,
data: "B",
intent: "set",
},
]);

assert({
items: ["B"],
});
});

it(`with intent "set" should notify with a "insert" update if no item exists at this position`, async () => {
const { root, assert, applyRemoteOperations, subscribe } =
await prepareIsolatedStorageTest<{ items: LiveList<string> }>(
[
createSerializedObject("root", {}),
createSerializedList("0:0", "root", "items"),
createSerializedRegister("0:1", "0:0", FIRST_POSITION, "A"),
],
1
);

const items = root.get("items");
items.delete(0);

const callback = jest.fn();

subscribe(items, callback, { isDeep: true });

applyRemoteOperations([
{
type: OpType.CreateRegister,
id: "0:2",
parentId: "0:0",
parentKey: FIRST_POSITION,
data: "B",
intent: "set",
},
]);

expect(callback).toHaveBeenCalledWith([
{
node: items,
type: "LiveList",
updates: [{ type: "insert", index: 0, item: "B" }],
},
]);
});

it("on existing position should give the right update", async () => {
const { root, assert, applyRemoteOperations, subscribe } =
await prepareIsolatedStorageTest<{ items: LiveList<string> }>(
Expand Down
Loading