Patch for array.pop() replaces "length"

[aleclarson]

Let's assume array = [ 0 ]. Should array.pop() result in { op: "remove", path: [ "0" ] } instead of { op: "replace", path: [ "length" ], value: 0 }?

I guess Immer is already "breaking" from the spec by making path an array and not /foo/bar, but you could argue replacing length isn't friendly to non-JS environments.

I assume the current behavior improves performance slightly.

https://codesandbox.io/s/0o1zx5226l

• Version: 1.7.2
• Occurs when using Proxies (use setUseProxies(true))
• Occurs in the ES5 implementation (use setUseProxies(false))

[mweststrate]

@aleclarson no meaning about it, both are semantically the same and length operations are probably unavoidable. For example, one could detect if only one item is removed, that a remove patch should be generated. But that might make splice quite unpredictable, and generate different types of patches in different situations.

If a patch is simple & small, I'm fine applying it; patch recording only guarantees that applying the same patches to the same state results in the same end state, not that it is the minimal or most optimal patch set :-). (Although the set is quite optimal actually in most cases)

[mweststrate]

Closing for inactivity (and since PR has been closed?) cc: @aleclarson

[davidye]

I don't think this was fixed?

@mweststrate @aleclarson

> produce([0], draft => { draft.pop(); }, (p) => { console.log(p); } )

[ { op: 'replace', path: [ 'length' ], value: 0 } ]

[aleclarson]

Yeah, this should be fixed. We don't want to stray from JSON patches unless necessary.

[davidye]

I've been using immer patches to synchronize a large JSON object between client (js) and server (elixir) - so that's great to hear.

[aleclarson]

To fix this, rebasing and testing #210 is "up for grabs" to anyone who wants to contribute ❤️

[agupta93]

@aleclarson Would love to contribute to the same. But would require a little bit of help

[aleclarson]

@mweststrate What was the rationale behind diverging from RFC 6902 in the first place?

[mweststrate]

@aleclarson Immer just intercepts writes to the object on all properties including length. If a pop results in a length update, than that is apparently how the JS engine has decided to implement .pop(). Those 'higher level concepts api calls' are not detected by Immer, which just trackes the actual mutations that are happening to the object. if splice(3, 4, 1, 2) moves in a lot of entries copied 4 places forward for example, that is what you would see in the batches. if instead does a full reassign of all entries, that is what you would see, etc. In other words, Immer has no control over this process and just report what is has seen with further analysis of what it reflects :)

So if .pop() would instead have resulted in a delete on the designated index, we would have gotten a remove patch, rather than a length update patch :). But that is apparently not how it is implemented. (very wisely, it would be super duper slow)

[aleclarson]

@mweststrate Sorry, I was trying to ask why Immer doesn't follow RFC 6902 exactly, not why you diverged in this particular scenario. 😅 I assume it's because you didn't think it was worth the extra effort. :P

[mweststrate]

Yeah, so the essence is: because we don't intercept _operations_, but _mutations_ instead, so we don't know that a `length = 4` assignment represents a pop or just a length increase until we would intercept operations.

…

On Wed, Jul 17, 2019 at 10:45 PM Alec Larson ***@***.***> wrote: @mweststrate <https://github.com/mweststrate> Sorry, I was trying to ask why Immer doesn't follow RFC 6902 exactly, not why you diverged in this particular scenario. 😅 I assume it's because you didn't think it was worth the extra effort. :P — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#208?email_source=notifications&email_token=AAN4NBGI7KIH2GGVZIA4T3DP76AG3A5CNFSM4FYZFPLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2GREFQ#issuecomment-512561686>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBHY3PAJ3BPG5ES6ZSLP76AG3ANCNFSM4FYZFPLA> .

[jsdevtom]

Yeah, so the essence is: because we don't intercept operations, but mutations instead, so we don't know that a length = 4 assignment represents a pop or just a length increase until we would intercept operations.

This is an inconvenience for me personally. I am following your blog post on how to distribute the state.

In my use case, I need to convert the patches to database commands to persist them. Why?:

By changing the draft inside the producer like so: draft.splice(draft.findIndex(todo => todo.id === "id1")/*= last index*/, 1): The new length in a replace patch is returned (if the item to be removed was the last one). The only way for me to find out which one was removed would be to convert the patch to a remove patch manually. With a remove patch, I have the id of the car that was removed and can thus be converted to a database command.

Even if the state was exactly the same on the client as in the database, I would have to read all the items from the database to apply the length replace patch. This could have a large, negative performance impact depending on the number of entities in the database.

Because setting the length of the array to a lower length of the array deletes the elements inside, I propose catching this case and returning a remove patch. I am all ears for criticisms in my approach, logic or anything I've said.

PS. Awesome library!

[jsdevtom]

@aleclarson

I guess Immer is already "breaking" from the spec by making path an array and not /foo/bar, but you could argue replacing length isn't friendly to non-JS environments.

At this point in the spec it states that elements from an array should be a remove op:

[
     { "op": "remove", "path": "/foo/1" }
]

Am I missing something or have I misunderstood? As far as I can tell, the spec specifies, that a remove should result in a remove and nothing else.

[aleclarson]

🎉 This issue has been resolved in version 3.3.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[Cooke]

Got this issue again after upgrading from v5 to v9