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.

Sign up for GitHub

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

fix: issue with array methods that re-assign indexes #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: issue with array methods that re-assign indexes #45

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8455a58
Fix issue with array methods that re-assign indexes
ahmedkandel Mar 27, 2020
436caf2
make unwrapProxy method more safe for read-only proxies and use it fo…
ahmedkandel Mar 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/reactive-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,16 @@ export class ReactiveProxyHandler {
return membrane.getProxy(value);
}
set(shadowTarget: ReactiveMembraneShadowTarget, key: PropertyKey, value: any): boolean {
const { originalTarget, membrane: { valueMutated } } = this;
const { originalTarget, membrane: { valueMutated, valueIsObservable, unwrapProxy } } = this;
const oldValue = originalTarget[key];
if (isArray(originalTarget) && valueIsObservable(value) && originalTarget.includes(unwrapProxy(value))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this change does makes sense, but I think it should be a lot more generic. Let me loop more folks into this because IIRC there are some history around this. /cc @ravijayaramappa @davidturissini

I believe defineProperty is doing the right thing by unwrapping the value in the descriptor when setting it into the original target. The set trap should do the same all the time by just simply unwrap the value before setting that up. The question is, what are the implications of such generalization.

value = unwrapProxy(value);
if (oldValue !== value) {
    originalTarget[key] = value;
    valueMutated(originalTarget, key);
} else {...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrapping the value all the time will cause some issues:

  1. It will allow injecting read-only proxy to an array to make it read-write:
const membrane = new ObservableMembrane();

const o = { foo: 'bar' };

const readOnly = membrane.getReadOnlyProxy(o);
const writeAndRead = membrane.getProxy({baz: []});

writeAndRead.baz.push(readOnly);
writeAndRead.baz[0].foo = 'qux';

writeAndRead.baz[0].foo will return 'qux' not 'bar'

  1. Pushing a proxy to the array will not be allowed and will get unwrapped:
const membrane = new ObservableMembrane();

const o = {
    x: 2,
    y: [
        {z:1}
    ]
};

const p = membrane.getProxy(o);

p.y.push(membrane.getProxy({z: 2}));

p.y[1] will yield an object but not a proxy

  1. If the original value of an array item is already a proxy for some use-cases the unwrapping will transform the original value when doing (shift, unshift, ...).

That's why I had to narrow the scope to array items re-assignment. Also by checking originalTarget.includes(unwrapProxy(value)) we make sure we conserve a proxy value if it does exist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahmedkandel thanks for bringing this up, now I remember a lot more about this. Ok, let me try to articulate my position on this:

  1. I don't think this is an array specific issue, I think it is a fundamental issue, it also affects objects of those objects have functions that carry mutation similars to those done by the array intrinsics.

  2. yes, readOnly proxies have particular semantics that have to be preserved, should not be that hard IMO.

Now, let me state the problem, the way I see it:

Lets use a code of colors, which I find easier to understand, in this library we have two colors: yellow and blue:

  • readOnly proxies and readAndWrite proxies should be considered yellow objects and arrays controlled by this library.
  • any other object in general (object, functions, arrays, etc.) should be consider blue.
  • yellow objects can be readOnly (RO) or readAndWrite (RW).

Operations to control by this membrane for RW:

a. Reflect.set(yellowRWObject, key, blueValue) as a result, a blueValue should be set into the target of the yellowRWObject (that target is another blueValue). (no unwrapping needed)
b. Reflect.set(yellowRWObject, key, yellowROValue) as a result, a yellowROValue should be set into the target of the yellowRWObject (that target is another blueValue). I call this a leaking escape hatch, which is a necessary evil to avoid making a RO value a RW value by all means. (no unwrapping needed)
c. Reflect.set(yellowRWObject, key, yellowRWValue) as a result, the target of the yellowRWValue should be set into the target of the yellowRWObject (that target is another blueValue). (unwrapping needed)

Operations to control by this membrane for RO Objects are not that important because they just throw for all cases.

What all this tell us, is that the only condition that needs to be added is for the case where a RO value is about to be set into a RW value as a property, in which case no unwrapping should be done (case b above), while for the rest, should require unwrapping attempts.

In terms of the code, let me adjust my proposal:

value = unwrapProxyOnlyIfReadAndWriteProxyIsGiven(value);
if (oldValue !== value) {
    originalTarget[key] = value;
    valueMutated(originalTarget, key);
} else {...

We can either create that new method (with a better name), or reuse something else. This will also have to be changed in unwrapDescriptor method, which is used by defineProperty trap to accommodate that as well since it is faulty as well today.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly @caridy, I was thinking about this implementation with a little change in (case c above).

When setting yellowRWValue, maybe the user has the intention to set a proxy yellowValue, not blueVlaue described in my previous comment (case 2 above). While I think it is rare and we can clarify in the documentation that it is not allowed to set() proxy values to a property as it will get unwarped. Also because get() will always return a proxy whenever the value is observable "membrane is applicable to an entire object graph" so no need for the user to do it by himself.

In this case, generalizing value unwrapping in ReactiveProxyHandler . set() and defineProperty() traps except for read-only proxy values yellowROValue is a better fix.

We can add a new method to ReactiveMembrane maybe we name it safeUnwrapValue that return unwrappedValue if not a yellowROValue.

Another approach is to add a condition to ReadOnlyHandler . get() trap

if (key === isReadOnlyProxy) {
    return true;
}

Then we can check it later if (value[isReadOnlyProxy]) ...

I think the new method approach is better. I will commit it so we could start discussing it.

// fix issue #44 using array methods that re-assign indexes
// like (splice, shift, unshift, sort, reverse)
// will get() re-assigned items wrapped with proxy.
// So it will replace originalTarget items with proxies.
// This to unwrap those proxies and assign the original value.
value = unwrapProxy(value);
}
if (oldValue !== value) {
originalTarget[key] = value;
valueMutated(originalTarget, key);
Expand Down