-
Notifications
You must be signed in to change notification settings - Fork 245
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.
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
LiveList.set #147
LiveList.set #147
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! I'm fully on board with the problem this new LiveList.set()
method solves. I did have a question about the implementation, specifically about the optional intent
flag, and have a slight worry that this may open up the way to attach more "edge cases" to the creation op messages in the future, and was wondering if this is the right time and place to add a new op type of this kind of operation instead. Curious to get your thoughts on this!
Nothing blocking, though — this is a great addition! 💯
opId: string, | ||
isLocal: boolean | ||
): ApplyResult; | ||
abstract _attachChild(op: CreateOp, isLocal: boolean): ApplyResult; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
parentId: string, | ||
parentKey: string, | ||
doc?: Doc, | ||
intent?: "set" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
Servers need to be updated to make the e2e tests pass
LiveList.set
What's the difference between
LiveList.set
andListList.delete
+LiveList.insert
?Replacing an element is similar to removing it and inserting another element at the same position, but there is a slight difference in "intent" that has implications in a multiplayer app.
Let's imagine a video app where you have a list of topics associated with a video, for example:
["Computer science", "Mathematics", "Literature"]
.User A decides that
"Literature"
is not an appropriate topic for this video and decides to replace it with"Physics"
. At the same time (or while offline), user B decides that"Literature"
is not an appropriate topic for this video and decides to replace it with"Quantum computing"
.If the app replaces an item using
delete
theninsert
, here is what's happening on the server.Initial state:
["Computer science", "Mathematics", "Literature"]
"Literature"
from user A =>["Computer science", "Mathematics"]
"Physics"
from user A =>["Computer science", "Mathematics", "Physics"]
"Literature"
from user B =>"Literature"
does not exist anymore, the server ignore this operation"Quantum computing"
from user B =>["Computer science", "Mathematics", "Physics", "Quantum computing"]
There are now 4 topics associated with the video. But if the intent were to replace
"Literature"
with another topic, it would be better to have only 3 topics associated with the video. To have either["Computer science", "Mathematics", "Physics"]
or["Computer science", "Mathematics", "Quantum computing"]
depending on which operation arrived last to the backend (Last Writer Win).This is what's happening with
LiveList.set
.Initial state:
["Computer science", "Mathematics", "Literature"]
["Computer science", "Mathematics", "Physics"]
"Quantum computing"
at position 2 from user B =>["Computer science", "Mathematics", "Quantum computing"]
Impact on
redux
andzustand
integrationOn the current version, when updating an immutable array from
["A", "B", "C"]
to["A", "D", "C"]
, the server receives adelete
+insert
, with this PR, it will receive aset
operation. It optimizes tuple updates, and the final state is always more consistent based on the intent.