Closed bogobogo closed 2 years ago
Thanks! Could you add a unit test that tests the method?
I would like to add
set
by index next. I see you are throwing an error there. What do you think about implementing it using splice?
This is a tricky one. In a distributed collaboration scenario, I'm not yet convinced "set" using splice is really safe. Because slice is a delete + insert operation, I think the following would happen if both Alice and Bob assign a new element to arr[2] at the same time:
// start:
arr = ["red", "green", "blue"]
// Alice:
arr[1] = "purple";
// translates to (remove arr[1], insert "purple" at position 1)
// Bob:
arr[1] = "orange";
// translates to (remove arr[1], insert "purple" at position 1)
When both changes sync, we'd end up with:
arr = ["red", "orange", "purple", "blue"]
With "insert" and "delete", and even "splice" names for operations this result makes sense. But when doing an assignment (arr[1] = x
), I think this is quite counterintuitive.
For this reason, until now I decided not to implement this method (I think it would cause quite some confusion and broken applications because devs won't understand the impact). What do you think?
Added simple tests for every. Let me know if you want to me to add anything else (while I am at it, I might add tests for find and some other methods with missing tests in future PR's)
As for set, that makes sense. I can see the issue https://github.com/yjs/yjs/issues/16 where this is being discussed in yjs.
Thinking through it, the options one has are:
1) Understand the consequences and decide to use splice anyway 2) Hold references to other shared structures and update those 3) Replace the whole array(?)
Am I missing anything?
I agree with the unexpected nature of set. I have two optional suggestions that might help here:
Something along the line of implementSetWithSplice()
or something like that.
What do you think?
- Simply a more descriptive error message with alternatives, since this is such a widely used operation in js/ts
I think this would make most sense. In the code, maybe add a comment that links to https://github.com/yjs/yjs/issues/16 and this issue to explain the decision.
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
packages/core/src/array.ts | 1 | 2 | 50.0% | ||
<!-- | Total: | 1 | 2 | 50.0% | --> |
Totals | |
---|---|
Change from base Build 1779211027: | 0.1% |
Covered Lines: | 534 |
Relevant Lines: | 626 |
@YousefED Sure, if I will get to it I will do it in a separate PR.
There are a few basic array methods missing in the current array proxy implementation,
every
is one of them so this is a small PR to add it.I would like to add
set
by index next. I see you are throwing an error there. What do you think about implementing it using splice?