Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Immutable Apply? #26

Open
curran opened this issue Apr 6, 2019 · 4 comments
Open

Immutable Apply? #26

curran opened this issue Apr 6, 2019 · 4 comments

Comments

@curran
Copy link

curran commented Apr 6, 2019

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
Copy link
Author

curran commented Apr 9, 2019

I discovered an actual implementation of this idea here WeTransfer@00d10c9

And another here (this one leverages immutability-helper) codedownio@e2f3976

@curran
Copy link
Author

curran commented Apr 10, 2019

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

@josephg
Copy link
Member

josephg commented Apr 10, 2019

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
Copy link
Author

curran commented Aug 1, 2019

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);
  });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants