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.

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

Add support for array.every #52

Merged
merged 2 commits into from Feb 22, 2022
Merged

Add support for array.every #52

merged 2 commits into from Feb 22, 2022

Conversation

bogobogo
Copy link
Contributor

@bogobogo bogobogo commented Feb 7, 2022

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?

@YousefED
Copy link
Owner

YousefED commented Feb 8, 2022

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?

@bogobogo
Copy link
Contributor Author

bogobogo commented Feb 8, 2022

Every

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)

Set

As for set, that makes sense. I can see the issue yjs/yjs#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?

Potential Improvements

I agree with the unexpected nature of set. I have two optional suggestions that might help here:

  1. Simply a more descriptive error message with alternatives, since this is such a widely used operation in js/ts
  2. Expose an API to explicitly choose what set does, either by calling a function, or by some config. If this API has not been called, than set will throw an error by default.

Something along the line of implementSetWithSplice() or something like that.

What do you think?

@YousefED
Copy link
Owner

  1. 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 yjs/yjs#16 and this issue to explain the decision.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1811245091

  • 1 of 2 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 82.197%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/core/src/array.ts 1 2 50.0%
Totals Coverage Status
Change from base Build 1779211027: 0.1%
Covered Lines: 534
Relevant Lines: 626

💛 - Coveralls

@bogobogo
Copy link
Contributor Author

@YousefED Sure, if I will get to it I will do it in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants