Could you add a test for creating external deep proxy?
I still don't see a clear reason why previous implementation with a map is not capable of doing so. What's the difference between of having instance specific map than instance specific symbol?

Could you add a test for creating external deep proxy?

There is a test in the commit c461b49

I still don't see a clear reason why previous implementation with a map is not capable of doing so. What's the difference between of having instance specific map than instance specific symbol?

Consider this. There are two ways of accessing an object:

• by reference. In JSONPatcherProxy, such way of accessing is used whenever you see tree. This way gives you original unproxified instance of the object, because that's how the set trap works.
• by parent and key. In JSONPatcherProxy, such way of accessing is used whenever you see tree[key]. This way gives you whatever the external world sees as the value, including the proxy made by the current instance of JSONPatcherProxy and any other proxy.

All places in JSONPatcherProxy where there was a need to use tree[key], used to use tree[key] as the key to a Map object. However, this only worked as long as the value at tree[key] was exactly the proxy object created by the current instance of JSONPatcherProxy. It the value was additionaly proxified by external code, it was detected by JSONPatcherProxy as a totally new object.

This is because in the ES Proxy world, the proxy instances have different identity than the original object:

var obj = {};
var p1 = new Proxy(obj, {});
var p2 = new Proxy(p1, {});
obj === p1; //false
p1 === p2; //false

The method used in this PR works by recognizing the object already proxified by an instance of JSONPatcherProxy not by keeping a reference to it, but by marking it from the inside:

var obj = {};
var meta = {};
var symb = Symbol("Symbol for getting the tree metadata from external access.");
obj[symb] = meta;

var p1 = new Proxy(obj, {});
var p2 = new Proxy(p1, {});
obj[symb] === p1[symb]; //true
p1[symb] === p2[symb]; //true

There is a test in the commit c461b49

Sorry my bad

Thanks, now I get why having Map of object references will not work.
But then doesn't keeping parent object reference shares the same problem? If it doens't maybe we could keep a map of parent, key tuples.

I'm a little bit picky, as I remember we used to have implementation that was using get trap to identify proxified object, and we discarded it for some reason (I suspect performance)

I made a benchmark for reads, see my last 4 commits.

You are right, this PR introduces about 25% performance degradation in reads. We should care about this, because performance in reads is already terrible compared to lack of proxy (see full results below).

Anyway, I think that a fix for the problem #35 is absolutely necessary. Do you have any other suggestions?

Before this PR

Observe and generate, small object (JSONPatcherProxy)
 Ops/sec: 94422 ±2.81% Ran 5965 times in 0.063 seconds.
Observe and generate (JSONPatcherProxy)
 Ops/sec: 1924 ±2.29% Ran 190 times in 0.099 seconds.
Primitive mutation (JSONPatcherProxy)
 Ops/sec: 480043 ±2.65% Ran 27032 times in 0.056 seconds.
Complex mutation (JSONPatcherProxy)
 Ops/sec: 6996 ±5.30% Ran 421 times in 0.060 seconds.
Serialization (JSONPatcherProxy)
 Ops/sec: 1252 ±0.77% Ran 66 times in 0.053 seconds.

After this PR

Observe and generate, small object (JSONPatcherProxy)
 Ops/sec: 94633 ±2.61% Ran 5925 times in 0.063 seconds.
Observe and generate (JSONPatcherProxy)
 Ops/sec: 2010 ±2.06% Ran 194 times in 0.097 seconds.
Primitive mutation (JSONPatcherProxy)
 Ops/sec: 414442 ±1.92% Ran 22812 times in 0.055 seconds.
Complex mutation (JSONPatcherProxy)
 Ops/sec: 6106 ±4.07% Ran 350 times in 0.057 seconds.
Serialization (JSONPatcherProxy)
 Ops/sec: 935 ±0.82% Ran 49 times in 0.052 seconds.

Full results after this PR

Observe and generate, small object (noop)
 Ops/sec: 12577915 ±0.20% Ran 641041 times in 0.051 seconds.
Observe and generate, small object (JSONPatcherProxy)
 Ops/sec: 92129 ±2.62% Ran 5823 times in 0.063 seconds.
Observe and generate, small object (fast-json-patch)
 Ops/sec: 69657 ±1.80% Ran 3888 times in 0.056 seconds.
Observe and generate (noop)
 Ops/sec: 160466 ±0.26% Ran 8364 times in 0.052 seconds.
Observe and generate (JSONPatcherProxy)
 Ops/sec: 1958 ±1.39% Ran 236 times in 0.121 seconds.
Observe and generate (fast-json-patch)
 Ops/sec: 1698 ±1.10% Ran 93 times in 0.055 seconds.
Primitive mutation (noop)
 Ops/sec: 2549711 ±0.60% Ran 130739 times in 0.051 seconds.
Primitive mutation (JSONPatcherProxy)
 Ops/sec: 403460 ±3.16% Ran 22659 times in 0.056 seconds.
Primitive mutation (fast-json-patch)
 Ops/sec: 7835 ±0.20% Ran 401 times in 0.051 seconds.
Complex mutation (noop)
 Ops/sec: 6551384 ±0.20% Ran 331525 times in 0.051 seconds.
Complex mutation (JSONPatcherProxy)
 Ops/sec: 6035 ±6.15% Ran 354 times in 0.059 seconds.
Complex mutation (fast-json-patch)
 Ops/sec: 7707 ±0.62% Ran 406 times in 0.053 seconds.
Serialization (noop)
 Ops/sec: 7206 ±0.41% Ran 375 times in 0.052 seconds.
Serialization (JSONPatcherProxy)
 Ops/sec: 948 ±0.35% Ran 50 times in 0.053 seconds.
Serialization (fast-json-patch)
 Ops/sec: 7101 ±0.40% Ran 373 times in 0.053 seconds.

Have you tried

But then doesn't keeping parent object reference shares the same problem? If it doesn't maybe we could keep a map of parent, key tuples.

I agree it's necessary to fix that problem over the performance, but we should do our best not to degrade it too much. Therefore if the idea of keeping a map of parent+key does not work, let's merge it.

I fixed the generate calls. Current numbers:

After this PR

Observe and generate, small object (JSONPatcherProxy)
 Ops/sec: 95913 ±2.54% Ran 5936 times in 0.062 seconds.
Observe and generate (JSONPatcherProxy)
 Ops/sec: 2008 ±1.49% Ran 210 times in 0.105 seconds.
Primitive mutation (JSONPatcherProxy)
 Ops/sec: 436081 ±0.48% Ran 22251 times in 0.051 seconds.
Complex mutation (JSONPatcherProxy)
 Ops/sec: 7526 ±0.37% Ran 387 times in 0.051 seconds.
Serialization (JSONPatcherProxy)
 Ops/sec: 916 ±0.29% Ran 47 times in 0.051 seconds.

Some numbers acutally went up, because before this commit, the queue of recorded patches was growing infinitely. Now, the queue is flushed after each repetition.

I pushed an additional test to check if double proxification by JSONPatcherProxy works as intended. It does work.

I am not merging this until we find it absolutely necessary for view binding.

One thing that I find very annoyning about this change is how it affects debugging. Any attempt to read a property from the observed object causes the debugger to step into the trapForGet.

I made a new PR that contains only the test refactorings from the current PR: #42

This PR was rebased on current master and is now available as a new PR #43