⏲ Implement object path tracking

[alshakero]

Explanation:

How do we proxify objects?

1. Create a new tree object.
2. Iterate all originalObject properties and proxify them one by one. Copy each proxified prop to tree.
3. proxify function would pass path to ES6 proxy traps (set and deleteProperty). Because it knows where is it in the object tree throughout traversing.

All done, now we have a proxified tree with a set of proxies that have set functions caching path inside them.

Looks fine.

Until

The project tree is mutated in a way that doesn't affect the deep proxy itself but changes its path. Like:

tree = [{value: 1}, {value: 2}]
tree.shift();

Now tree is [{value: 2}], and 2 thinks its path is /1/value.

tree[0].value = 3;

// would emit {op: replace, path: /1/value, value: 3}. Which is bad.

This PR makes JP keep track of all proxies' paths and updates them if needed after every mutation.

Fixes: https://github.com/Palindrom/JSONPatcherProxy/issues/9 and more.

[alshakero]

@warpech this is not the primary but a fast solution. It's much much faster than deep cloning. It really took some deep thinking and crazy focus to visualize the whole thing and come up with this.

[alshakero]

@tomalec any comments on this? I fixed https://github.com/Palindrom/Palindrom/issues/73 and would like to release Palindrom 3.0.2. Would be cool to release this to be bundled in Palindrom 3.0.2 to save a version number and Level1 PR hassle.

[alshakero]

@tomalec thank you so much. Looks much better now.

[alshakero]

Is this good to go?

[warpech]

No comments from me at least, I am not sure if @tomalec has his points answered though?

[alshakero]

I am not sure if @tomalec has his points answered though?

To the best of my knowledge, they all are.

[tomalec]

https://github.com/Palindrom/JSONPatcherProxy/pull/10#discussion_r131332864

[alshakero]

I did move the comment above the function call, I don't think we should move it above the function itself. It's for the line, not the function. https://github.com/Palindrom/JSONPatcherProxy/blob/40350a6f7aa36dff4e361f4686fb48be1c2b1809/src/jsonpatcherproxy.js#L10-L14

[tomalec]

I did move the comment above the function call, I donn't think we should move it above the function itself. It's for the line, not the function.

I'm talking about banner:

'use strict';
/**
 * A helper function that calls Reflect.set.
 * It is utilized to re-populate path history map after every `Reflect.set` call.
 * @param {JSONPatcherProxy} instance JSONPatcherProxy instance
 * @param {Object} target target object
 * @param {string} key affected property name 
 * @param {any} newValue the new set value
 */
function ownReflectSet(instance, target, key, newValue) {
  const result = Reflect.set(target, key, newValue);
  /* we traverse the new (post-apply) object and update proxies paths accordingly */
  instance._resetCachedProxiesPaths(instance.cachedProxy, '');
  return result;
}
/*!
 * https://github.com/PuppetJS/JSONPatcherProxy
 * JSONPatcherProxy version: 0.0.5
 * (c) 2017 Starcounter 
 * MIT license
 */

/** Class representing a JS Object observer  */
const JSONPatcherProxy = (function() {

to

/*!
 * https://github.com/PuppetJS/JSONPatcherProxy
 * JSONPatcherProxy version: 0.0.5
 * (c) 2017 Starcounter 
 * MIT license
 */

'use strict';
/**
 * A helper function that calls Reflect.set.
 * It is utilized to re-populate path history map after every `Reflect.set` call.
 * @param {JSONPatcherProxy} instance JSONPatcherProxy instance
 * @param {Object} target target object
 * @param {string} key affected property name 
 * @param {any} newValue the new set value
 */
function ownReflectSet(instance, target, key, newValue) {
  const result = Reflect.set(target, key, newValue);
  /* we traverse the new (post-apply) object and update proxies paths accordingly */
  instance._resetCachedProxiesPaths(instance.cachedProxy, '');
  return result;
}

/** Class representing a JS Object observer  */
const JSONPatcherProxy = (function() {

Also you made ownReflectSet totally global and I think that is not necesarry.

[alshakero]

Legit point.

[tomalec]

https://github.com/Palindrom/JSONPatcherProxy/pull/10/files#r131217191

[warpech]

Great, thanks for such a deep review that I simply could not make.

I just thought it's limitation we plan to remove in long term. Also, such change could be done by palindrom, as well.

That's nice to have, I agree, but right now we are fixing a high priority issue, so we should fix, add tests, make a note about the limitations and move forward.

[alshakero]

Great, thanks for such a deep review

Totally agree, the code looks much better after Tomek's reviews.

[alshakero]

I merged without updating docs, I'll bundle Palindrom, PR to Level1, and then PR for docs.