automerge / automerge-classic

A JSON-like data structure (a CRDT) that can be modified concurrently by different users, and merged again automatically.
http://automerge.org/
MIT License
14.75k stars 466 forks source link

adds support for setting arrays length #415

Closed nathanfaucett closed 3 years ago

ept commented 3 years ago

Hi @nathanfaucett, thanks for this contribution, which looks good to me. I just wanted to raise one point: when growing the size of an array, this patch initialises the new elements with null, whereas standard JavaScript behaviour is to initialise with undefined. Indeed we can't use undefined since Automerge doesn't allow that value (and JSON doesn't either); still, Automerge is deviating from the standard JS semantics. Is that acceptable, on the basis that allowing length to be changed but filling with nulls is still closer to standard JS semantics than not allowing length to be changed at all?

Also, I'd be curious to hear a bit about your use case for assigning length.

nathanfaucett commented 3 years ago

@ept I had the same thought about nulls, but since setting the length is something many js libraries do and the reason I ran into this problem I thought it was worth a PR at least. My use case is using deep-diff to apply quill-delta changes from the new array to the automerge array instead of just overriding the whole thing, this might be my lack of understanding of automerge, my thought was that a full override would result in more changes to the automerge document than just applying the differences.

ept commented 3 years ago

Okay, that makes sense. Thanks for the explanation. Yes, it's better to apply the minimum diff possible, because Automerge doesn't perform any diffing of its own, and replacing the whole document would mean you don't get any sensible conflict merging behaviour. I've merged this PR, thanks for your contribution!

nathanfaucett commented 3 years ago

@ept related to this change and use case, automerge throws on setting outside the length of an array here which is not the default javascript array behavior, I have made some changes in my fork to handle this but wanted to check if there is a reason for not allowing this before I created another PR.

ept commented 3 years ago

Merged your other PR #416, thanks @nathanfaucett!