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

make it so you can combine patch that change the same properties #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FranzPoize
Copy link

I was working on making an undo stack and I needed to combine patch that operate on the same properties. Using the old combine the change in the new patch would overwrite the old patch mutations.

So here is a proposed changed to allow combine to merge patches which change the same properties.

@elierotenberg
Copy link
Owner

I might not understand what is it you're trying to accomplish, since the original Remutable allows successive mutations on the same property to be combined. However, the intermediate mutation is lost. If you want to keep your undo stack clean, you should not combine patches, but apply them one by one (or keep track of uncombined patches).

In addition, the proposed modification doesn't pass the test (maybe you have forgot to run gulp; node dist/test to run the test).

Please feel free to update this P/R for clarifications :)

@FranzPoize
Copy link
Author

Sorry for not testing it.

Anyway I have a state which is updated on every mouse move event and to avoid having a shitty granularity on the undo stack I don't add every patch generated on these mouse move event to the stack. I don't use a diff because i want to be able in the future to update the undo stack on a criteria between the new value and the first value of the current batch of patch.

As for the patch combine. A combined patch cannot be reverted because the f value of the mutations are replaced by the f value of the last patch mutations.

Here is an example :

var remut = require('remutable');
var yo = new remut({});
var patch1 = yo.set('1',{hello:'world'}).commit();
var patch2 = yo.set('1',{hello:'youpi'}).commit();
var Patch = remut.Patch;
var patch3 = Patch.combine(patch1,patch2);
patch3.toJSON()
'{"m":{"1":{"f":{"hello":"world"},"t":{"hello":"youpi"}}},"f":{"h":-1549353149,"v":0},"t":{"h":-461136177,"v":2}}'
patch1.toJSON()
'{"m":{"1":{"t":{"hello":"world"}}},"f":{"h":-1549353149,"v":0},"t":{"h":2105508812,"v":1}}'

For patch3 to be revertible from v2 to v0 its f mutation values should be undefined.

var patch4 = yo.set('1',"randomStuff").commit();
var patch5 = Patch.combine(patch2,patch4);
patch4.toJSON()
'{"m":{"1":{"f":{"hello":"youpi"},"t":"randomStuff"}},"f":{"h":-461136177,"v":2},"t":{"h":1230918272,"v":3}}'
patch2.toJSON()
'{"m":{"1":{"f":{"hello":"world"},"t":{"hello":"youpi"}}},"f":{"h":2105508812,"v":1},"t":{"h":-461136177,"v":2}}'

Same here the f value should be {hello:"world"} not {hello:"youpi"} for the patch to be revertible.

Maybe I'm mistaking the use of combine, but this seems counterintuitive.

And my title is not good at all I see it now :p

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

Successfully merging this pull request may close these issues.

2 participants