Throw when array and object deletion values do not match current version

[alecgibson]

This change adds validation to array and object deletion. If the values provided in either ld or od do not match the current value, then apply will throw. It will also throw if oi overwrites an existing value without providing od.

The primary motivation of this change is to ensure that all submitted ops remain reversible. At the moment, the following series of ops is possible:

• start with { foo: 'bar' }
• apply this op: { p: ['foo'], oi: 'baz' }
• ...resulting in { foo: 'baz' }
• invert the previous op: { p: ['foo'], od: 'baz' }
• and apply the inverted op: {}

When I apply, invert and apply, I should always wind up where I started, but in this case I clearly do not.

By ensuring that the od matches the current value, we make sure that all ops remain reversible.

Deep equality adds a dependency on fast-deep-equal.

[alecgibson]

This should address the issue I raised on this thread: https://github.com/ottypes/json0/issues/16#issuecomment-404566959

[josephg]

Hi! Thanks for your PR! Sorry its taken so long for anyone to get back to you on this.

I like this change; but I don't want to break anyone's code. Enforcing this would be a breaking change in this library. I'd like to have some sort of "strict mode" which actually checks this stuff, rather than just checking all the time.

It might instead be better to have a method which accepts a document and an operation and canonicalizes the operation, filling in the op remove fields based on the contents of the document.

@nornagon do you want to chime in on this?

[alecgibson]

Happy to hide this behind a flag if you want it non-breaking. What would be the best way to do that? Also, for what it's worth, I've been use this change in a fork on a production environment for a while now, so it's also pretty battle-tested.

[pypmannetjies]

Any feedback on the above suggestion?

[alecgibson]

@josephg / @nornagon is this PR good to go?

[nornagon]

Hm, I'm a little hesitant here as this adds a possibly significant performance issue, and further, adds a dependency, which we are hitherto without.

Possible alternative: check === only? I don't see a good use case for constructing delete ops with anything other than a direct reference from the snapshot.

[alecgibson]

We can't use === because by the time the op has been serialized, sent over the wire, deserialised, transformed and even cloned within json0 itself, any references will have been well and truly destroyed.

I'm not sure I understand the aversion to adding dependencies; deep equality is a hard problem, and libraries are there to help us solve hard problems. Admittedly as-is, it would increase the package size by ~17%, but the actual bones of the thing are small and (if we remove ES6/React-related code), we could even copy the whole function into json0 at the cost of ~40 lines of code if you're particularly concerned about package size (the library is under MIT).

In terms of performance, I'm happy to do any benchmarking you'd need. The library itself already boasts some impressive benchmarks, and we've been using json0 with deep equality checking in Production for 3 years now and we've not run into any noticeable issues.

If this functionality is truly something you don't want in json0, that's fine. I just think it's a shame, given:

• there are todos in the code to add this functionality
• without this, the type isn't guaranteed to be invertible
• in my personal experience, some frontend JS frameworks have been quite naughty about mutating objects they don't own (I know AngularJS 1 used to add $$hash keys to our ShareDB doc data, which we had to make sure we stripped out when setting od)
• these checks have helped us catch a bunch of bugs, both in our own code, as well as the path issue I recently raised

[nornagon]

17%? that's a bit surprising, the code of fast-deep-equal looks smaller than that. + it looks like if we don't include the react/es6 versions of it, we won't get any unnecessary checks for weird react junk or Sets/Maps/etc. which we don't expect in any case.

If you've used this in production and both (a) haven't found performance issues with it and (b) it's discovered real bugs for you, then that seems like good justification to merge this. Just want to get clarification on the code size thing :)

[alecgibson]

17% is hopefully a worst-case — I based it on the unpackaged size quoted on npm.com (13kB) without any tree-shaking. It's hard to actually say the "real" impact, just because it depends on the tree-shaking of the consumer (eg if they're already using fast-deep-equals, then there's no impact at all).

(As a side-note, we're currently packaging the test/ folder, which we can exclude if we want to get the package size down further)

[alecgibson]

(NB: I just noticed that the fast-deep-equal package has received a major bump since I first opened this PR, so I've bumped it. The breaks don't affect us).