ottypes / json0

Version 0 of the JSON OT type
445 stars 62 forks source link

Immutable Apply? #26

Open curran opened 5 years ago

curran commented 5 years ago

It would be really great if the library could apply ops such that instead of mutating the original data, a new object is created (with shared structure with the previous data object).

Related:

curran commented 5 years ago

I discovered an actual implementation of this idea here https://github.com/WeTransfer/json0/commit/00d10c9a0bcfd5ae16314006fa0cfc5c6cf29351

And another here (this one leverages immutability-helper) https://github.com/codedownio/json0/commit/e2f3976a1b20edf98468d089bcb9dea42955a820

curran commented 5 years ago

It turns out json1 implements immutable apply https://github.com/ottypes/json1/blob/master/test/immutable.coffee#L7

josephg commented 5 years ago

Yeah I did it that way there because I totally agree with you - apply should be immutable.

I'd support a PR changing apply to work this way. I don't think we need to pull in an immutable library to do it. Just making a shallow copy on the way down to the modified object should be enough. I'm not sure if inserted data should be cloned in or just moved in. I think if apply is immutable, taking a reference should be fine. (It should be one or the other.)

In the tests the best way to verify this is to recursively call Object.freeze() on all the input data before calling apply. That way if anything is accidentally mutated, the tests will fail.

curran commented 4 years ago

FWIW I managed to solve this by monkey patching apply and using immer, like this:

import produce from 'immer';
const originalApply = json0.type.apply;
json0.type.apply = (snapshot, op) =>
  produce(snapshot, draftSnapshot => {
    originalApply(draftSnapshot, op);
  });