Generate patches programatically & patch grouping (was: Invalid Patch Content order in Firefox)

[ghost]

The content order of an patch differ between Firefox and Chrome

_Javascript code snippet_

this.templateInstance.model.AuthToken$ = token;
this.templateInstance.model.SignIn$ = null;  // This is an operation handler

_Chrome:_ (Correct order)

[
    {
        "op": "replace",
        "path": "/menu/SignInApp/AuthToken$",
        "value": "m8VuWg_7coG8ikZdlgLuCA2"
    },
    {
        "op": "replace",
        "path": "/menu/SignInApp/SignIn$",
        "value": null
    }
]

_FireFox:_ (Incorrect order)

[
    {
        "op": "replace",
        "path": "/menu/SignInApp/SignIn$",
        "value": null
    },
    {
        "op": "replace",
        "path": "/menu/SignInApp/AuthToken$",
        "value": "SkVDaOCX2zxW0J0NQB081A2"
    }
]

[warpech]

This is actually a problem with JSON Patch shim but I think it is impossible to fix.

In Firefox/IE/Safari, the changes are observed using a shim that triggers a dirty check on time intervals and after mouse & keyboard events.

The shim tries to detect the changes by comparing the current state of the object properties with the last saved copy of the object. When more than 1 change is found, it is impossible to detect what was the actual order of changes in a single time interval.

[ghost]

:+1: i think the order is important. Imagine you change the "Name" property then invoke the "Save" operation.. then you may not get the right "Name"

[warpech]

@mattiasmak has reported a similar problem with his app few days ago.

Let's reopen this issue to collect some thoughts.

The only idea that I have is to do something like:

var that = this;
this.templateInstance.model.AuthToken$ = token;
this.templateInstance.model.generate(); //triggers JSON Patch generate
setTimeout(function() {
  that.templateInstance.model.SignIn$++;
}, 1);

But how to do that in HTML?

<button value="{{model.SignIn$::click}}" model="{{model}}" onclick="model.generate(); ++this.value">Sign in</button>

Or, using a static method:

<button value="{{model.SignIn$::click}}" onclick="Puppet.generate(); ++this.value">Sign In</button>

[mattiasmak]

Is a helper method in puppet a solution? Send all properties to the method and let puppet create the patch in the correct order?

puppet.triggerpatch({Somedata$:1, Someotherdata$:2, Evenmoredata$:'data'});

Or do you want puppet.js to be as clean as possible?

[mattiasmak]

Or we just use a | seperated string for now... But I don't like it. I see Polyjuice as a framework and this should work out of the box without doing hacks and stuff. Sorry for being grumpy.

[mtornwall]

Darn, this just bit me too. :-)

While it's unfortunate that we cannot guarantee that the program write order is reflected in the patch, it isn't an insurmountable obstacle.

What we need is something akin to a memory barrier - a way of forcing Puppet to make sure that all writes that occurred before the barrier have been applied (notice the vague wording) after the barrier:

// Modify some properties that require no particular ordering.
model.Property1$ = "foo";
model.Property2$ = "bar";

// Writes to Property1$ and Property2$ may or may not have
// registered yet, and if they have, they could be in any order.
Puppet.barrier();

// Writes to Property1$ and Property2$ will definitely have taken place now.
model.Submit$++;

Without being familiar with the specifics of the PuppetJS implementation, it seems to me that a "simple" and correct implementation strategy would be for the barrier to force a dirty check, then generate and enqueue (or even send) a patch. Perhaps this was what you were getting at with the generate() function, @warpech? (Though I don't see why a setTimeout call is necessary; IMO the barrier should work synchronously)

Writes following the barrier would be recorded in a new patch, which (if this makes sense from an optimization standpoint) could still be bundled in the same message as the pre-barrier patch.

---

A completely different approach would be to reorder the writes consistently, perhaps by sorting the keys alphabetically. That way, I at least I can be sure that a change to model.Xyzzy is sent after a change to model.Foo.

Honestly, both of these solutions are at odds with developer intuition... I just don't know which one sucks less. :)

[tomalec]

I'm afraid that as Object.observe was deprecated, it may hit us more often :(

I really like the @mtornwall Barier approach. So we could have methods to:

• .touch - just calls dirty-check, to gather all already made changes, and put proceeding JSON Patch operation objects after
• .sync - .touch and sync with server-side
• .async - Similar to Polymer, just a sugar for @warpech setTimeout that will also make sure that already made patches were douched before callback execution (method names to be decided)

The question is whether we should make it:

1. instance puppet.method() - but then it would be hard to use it in HTML and with bindings to nested nodes
2. static Puppet.method() - but should it the execute it for all instances, all connections, and all models? - performance hurts, asking for trouble with other debugging unpredictable changes
   1. Puppet.method(model) - easier than first, cheaper that second,
   2. Puppet.method(node_somewhere_in_model) - easier than first, cheaper that second, traversing tree may cost.

[mtornwall]

@tomalec Great idea with the touch/sync/async function set; I think it's preferable to just the barrier I proposed above.

Regarding static vs instance I really prefer instance (despite how it appears in my example :-)). Perhaps Puppet could augment the model object with functions to do this? Alternatively, the model could have a property through which the Puppet instance could be accessed?

model.__puppet.touch();

[tomalec]

I do not like adding too many properties and methods, not to polute users space, and not to have too much about GC, there is already parent property, so in worst case we could add such property on root level. 12 sty 2016 14:28 "Martin Törnwall" [email protected] napisał(a):

@tomalec https://github.com/tomalec Great idea with the touch/sync/async function set; I think it's preferable to just the barrier I proposed above.

Regarding static vs instance I really prefer instance (despite how it appears in my example :-)). Perhaps Puppet could augment the model object with functions to do this? Alternatively, the model could have a property through which the Puppet instance could be accessed?

model.__puppet.touch();

— Reply to this email directly or view it on GitHub https://github.com/PuppetJs/PuppetJs/issues/29#issuecomment-170911614.

[mtornwall]

@tomalec Yeah, you're right. Perhaps Puppet.method(model) is the best choice, then? It's pretty easy to use, doesn't pollute the model object and has clear semantics. :+1:

[mtornwall]

Any update on this? Lately I've been getting diffuse reports suggesting that this issue occurs more frequently than expected.

[warpech]

@mtornwall No, still to separate batches of patches you should better trigger a mouseup event, as described here: https://github.com/PuppetJs/PuppetJs#generating-patches-based-on-local-changes

[mtornwall]

@warpech The problem with this approach is that it is still asynchronous. I don't really know when the mouseup event will be processed. By contrast, the barrier approach I proposed would be synchronous and therefore guarantee a partial write ordering across the barrier.

Edit: After a bit of research it seems that EventTarget.dispatchEvent is usually implemented synchronously, i.e., it doesn't involve the event loop. While this means that the event approach is likely to work on any modern browser, I'd much prefer a solution that doesn't depend on implementation-defined behavior.

[miyconst]

Just now me and @ubbeK were hit by this issue. A client side patch was sent to server too late, by that time the modified object was removed from the view-model and server fails.

So, I +1 this issue.

The scenario we have is:

• User clicks a button.
• The onclick listener executed.
• Local view-model changed.
• PuppetJs observes and sends changes.
• Server responses with another patch.
• Polymer observer executed.
• The observer changes local view-model.
• PuppetJs waits for a user action.

[malx122]

We have been hit a LOT by this latetly. Everytime there is two or more changes to the model they are sent in the REVERSE order of how they are found, kind of not so intuitive. Normally when I design a JSON on the serverside I finish with the "buttons" executing everything. But lets say that I have a text that should be sent on ok. Then the text (that triggers on lose focus) are sent after the ok-button if not the user click somewhere else first.

[warpech]

Today there was a similar problem in Starcounter: https://github.com/Starcounter/Starcounter/issues/4044

[dan31]

Looks to be a decently old issue. @tomalec, what do you think about this one in 2017?

[warpech]

I have added it to goals 2017 in September/October (as PuppetJs maintenance/fixes). This is very important issue, but others are more prioritised.

[alshakero]

@warpech isn't this going to be solved when we switch to proxies?

[warpech]

You are right, the proxy solves the problem of observing the changes in wrong order, which is what the OP says in https://github.com/PuppetJs/PuppetJs/issues/29#issue-41708810

But there is also another problem. Consider the following code:

puppet.obj.FirstName$ = "Jim";
puppet.obj.LastName$ = "Carrey";
puppet.obj.Save$ = 1 + puppet.obj.Save$;

This will be detected by the proxy as 3 distinct changes, right?

Will the PuppetJs send this as 3 separate sequences with a single patch, or as 1 sequence with three patches? I guess the former is easier to achieve, but actually there must be a way to also enforce the latter.

[alshakero]

@warpech I see. I guess it's easily doable. We can delay sending without losing any data.

[warpech]

I think that good ideas are here: https://github.com/PuppetJs/PuppetJs/issues/29#issuecomment-170904351. But I agree, let's get back to this after proxies.

[tomalec]

https://github.com/Palindrom/Palindrom/issues/128#issuecomment-291535681 shows that we need it at the same time we have release proxies.

I thought about something closer to what we have on server-side:

var node = root.some.path.deeper;
palindrom.batch/transaction(function(root){
    for(i = 0; i < 100; i++){
        node.arr[i] = 1;
    }
});

[warpech]

I am afraid that if the data change is performed asynchronously by some framework, we might be not possible to wrap changes in a function. Maybe we also should provide a flattened version - palindrom.batchStart, palindrom.batchEnd.

Also, how do you obtain the palindrom instance? Do you get the reference from juicy-html?

[alshakero]

@warpech that's was my preliminary idea. I also have another idea that can be the best but might force me re-do the tests 😢

Since until today we assume that jsonpatch is async, we can leverage this assumption and make proxies async too.

Instead of calling the callback instantly when a change occurs. We do this:

 // => a change happened 
patches.push(patch)
globalTimeout && clearTimeout(globalTimeout);
globalTimeout = setTimeout(() => emitPatches(patches), 1)

And by this, we wait for any blocking changes to happen and then emit. Since JS is single threaded.

[tomalec]

If data changes are done via automagic framework, it will behave as usual - send patch for each change. If developer would like to do some advanced binding he/she would have to do it manually. As such batched changes may also apply to data-binding framework, and each framework may support (or not) it in different manner.

Also, how do you obtain the palindrom instance? Do you get the reference from juicy-html?

Via document.querySelector('palindrom-client');

[tomalec]

Since until today we assume that jsonpatch is async, we can leverage this assumption and make proxies async too.

I would really like to get away of unnecessarily complicated flows.

[warpech]

Since until today we assume that jsonpatch is async, we can leverage this assumption and make proxies async too.

I was thinking about this kind of throttling too, but let's try to avoid this unless forced to. One thing that I will dislike for sure - I don't want Palindrom to be sometimes sync, sometimes async. It should be consistent in that manner.

[warpech]

Putting this to freezer because as of Palindrom 3.0.0, patches are generated as real time, not with dirty checking. Patch grouping should be less needed now.

[warpech]

Related: https://github.com/Palindrom/JSONPatcherProxy/issues/7#issuecomment-334085871