search

ottypes / json0

Version 0 of the JSON OT type
449 stars 64 forks source link

Additional validation for operations + shallow copies when applying operations #19

Closed ryanwe closed 6 years ago

ryanwe commented 8 years ago

This library is awesome and has been super helpful! In the course of using it, we found it helpful to add some validation to ensure that the operations being submitted are valid and fail fast. This means validating that the ld or od object is actually the expected object being deleted, range checks for l* operations, and that oi operations include od when they are overwriting/deleting an existing object.

This PR also includes a change to clone the entire operation instead of rebuilding it from scratch. We found places where attaching additional properties to an operation was helpful, and we want to maintain those properties after transforming the operations.

We use webpack (and sometimes browserify) to include this library on the client side and are able to require('assert') but if that doesn't work, I can add a custom assert.deepEqual() function to the code.

If this is considered a breaking change (due to the new errors and assertions), I can bump the version number too.

Like I said, this library has been great to work with and I'm happy to contribute back. Let me know if these changes look good or if they require additional modification.

Thanks!

ryanwe commented 7 years ago

Cool. Tests are passing which also means better node and browser compatibility. 👍

alecgibson commented 6 years ago

@josephg what's happening with this PR? I think these checks are hugely valuable.

josephg commented 6 years ago

Wow; there is a lot of great stuff here. I wish I merged a lot of this ages ago - my focus has been on newer, green-fields stuff. For what its worth, I agree that require('assert') is probably fine these days since just about everyone uses browserify / webpack / rollup / etc.

I'm not going to take this PR for two reasons:

I'd love to merge most of this. But I'm going to close this issue as-is. Feel free to re-open individual PRs with the changes and we can talk about them. I know its a pain. I'm sorry. But I don't want to blindly take responsibility for this code as-is.

Going forward it'd be nice to get some more committers direct access to this repo. Once I merge a few commits I'll be happy to just give you commit access and you can do the rest yourself