fix: issue #20 - supporting access to value via descriptor (#22)

* fixes #20: supporting access to value via descriptor, and setting new values via descriptor.
salesforce · Sep 16, 2018 · 4bd4751 · 4bd4751
1 parent 35ad3a4
commit 4bd4751
Show file tree

Hide file tree

Showing 6 changed files with 163 additions and 28 deletions.
diff --git a/src/reactive-handler.ts b/src/reactive-handler.ts
@@ -12,29 +12,28 @@ import {
     getOwnPropertyNames,
     getOwnPropertySymbols,
     preventExtensions,
+    hasOwnProperty,
 } from './shared';
 
 import {
     ReactiveMembrane,
     ReactiveMembraneShadowTarget,
+    wrapDescriptor,
 } from './reactive-membrane';
 
+function wrapValue(membrane: ReactiveMembrane, value: any): any {
+    return membrane.valueIsObservable(value) ? membrane.getProxy(value) : value;
+}
+
 // Unwrap property descriptors
 // We only need to unwrap if value is specified
 function unwrapDescriptor(descriptor: PropertyDescriptor): PropertyDescriptor {
-    if ('value' in descriptor) {
+    if (hasOwnProperty.call(descriptor, 'value')) {
         descriptor.value = unwrap(descriptor.value);
     }
     return descriptor;
 }
 
-function wrapDescriptor(membrane: ReactiveMembrane, descriptor: PropertyDescriptor): PropertyDescriptor {
-    if ('value' in descriptor) {
-        descriptor.value = membrane.valueIsObservable(descriptor.value) ? membrane.getProxy(descriptor.value) : descriptor.value;
-    }
-    return descriptor;
-}
-
 function lockShadowTarget(membrane: ReactiveMembrane, shadowTarget: ReactiveMembraneShadowTarget, originalTarget: any): void {
     const targetKeys = ArrayConcat.call(getOwnPropertyNames(originalTarget), getOwnPropertySymbols(originalTarget));
     targetKeys.forEach((key: PropertyKey) => {
@@ -46,7 +45,7 @@ function lockShadowTarget(membrane: ReactiveMembrane, shadowTarget: ReactiveMemb
         // could change sometime in the future, so we can defer wrapping
         // until we need to
         if (!descriptor.configurable) {
-            descriptor = wrapDescriptor(membrane, descriptor);
+            descriptor = wrapDescriptor(membrane, descriptor, wrapValue);
         }
         ObjectDefineProperty(shadowTarget, key, descriptor);
     });
@@ -145,16 +144,22 @@ export class ReactiveProxyHandler {
             return desc;
         }
         const shadowDescriptor = getOwnPropertyDescriptor(shadowTarget, key);
-        if (!desc.configurable && !shadowDescriptor) {
+        if (!isUndefined(shadowDescriptor)) {
+            return shadowDescriptor;
+        }
+        // Note: by accessing the descriptor, the key is marked as observed
+        // but access to the value, setter or getter (if available) cannot observe
+        // mutations, just like regular methods, in which case we just do nothing.
+        desc = wrapDescriptor(membrane, desc, wrapValue);
+        if (!desc.configurable) {
             // If descriptor from original target is not configurable,
             // We must copy the wrapped descriptor over to the shadow target.
             // Otherwise, proxy will throw an invariant error.
             // This is our last chance to lock the value.
             // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/handler/getOwnPropertyDescriptor#Invariants
-            desc = wrapDescriptor(membrane, desc);
             ObjectDefineProperty(shadowTarget, key, desc);
         }
-        return shadowDescriptor || desc;
+        return desc;
     }
     preventExtensions(shadowTarget: ReactiveMembraneShadowTarget): boolean {
         const { originalTarget, membrane } = this;
@@ -174,13 +179,13 @@ export class ReactiveProxyHandler {
         // if the descriptor has a value, as opposed to getter/setter
         // So we can just check if writable is present and then see if
         // value is present. This eliminates getter and setter descriptors
-        if ('writable' in descriptor && !('value' in descriptor)) {
+        if (hasOwnProperty.call(descriptor, 'writable') && !hasOwnProperty.call(descriptor, 'value')) {
             const originalDescriptor = getOwnPropertyDescriptor(originalTarget, key) as PropertyDescriptor;
             descriptor.value = originalDescriptor.value;
         }
         ObjectDefineProperty(originalTarget, key, unwrapDescriptor(descriptor));
         if (configurable === false) {
-            ObjectDefineProperty(shadowTarget, key, wrapDescriptor(membrane, descriptor));
+            ObjectDefineProperty(shadowTarget, key, wrapDescriptor(membrane, descriptor, wrapValue));
         }
 
         valueMutated(originalTarget, key);

diff --git a/src/reactive-membrane.ts b/src/reactive-membrane.ts
@@ -6,6 +6,7 @@ import {
     isUndefined,
     getPrototypeOf,
     isFunction,
+    hasOwnProperty,
 } from './shared';
 import { ReactiveProxyHandler } from './reactive-handler';
 import { ReadOnlyHandler } from './read-only-handler';
@@ -68,6 +69,31 @@ const defaultValueMutated: ReactiveMembraneMutationCallback = (obj: any, key: Pr
 };
 const defaultValueDistortion: ReactiveMembraneDistortionCallback = (value: any) => value;
 
+export function wrapDescriptor(membrane: ReactiveMembrane, descriptor: PropertyDescriptor, getValue: (membrane: ReactiveMembrane, originalValue: any) => any): PropertyDescriptor {
+    const { set, get } = descriptor;
+    if (hasOwnProperty.call(descriptor, 'value')) {
+        descriptor.value = getValue(membrane, descriptor.value);
+    } else {
+        if (!isUndefined(get)) {
+            descriptor.get = function() {
+                // invoking the original getter with the original target
+                return getValue(membrane, get.call(unwrap(this)));
+            };
+        }
+        if (!isUndefined(set)) {
+            descriptor.set = function(value) {
+                // At this point we don't have a clear indication of whether
+                // or not a valid mutation will occur, we don't have the key,
+                // and we are not sure why and how they are invoking this setter.
+                // Nevertheless we preserve the original semantics by invoking the
+                // original setter with the original target and the unwrapped value
+                set.call(unwrap(this), membrane.unwrapProxy(value));
+            };
+        }
+    }
+    return descriptor;
+}
+
 export class ReactiveMembrane {
     valueDistortion: ReactiveMembraneDistortionCallback = defaultValueDistortion;
     valueMutated: ReactiveMembraneMutationCallback = defaultValueMutated;

diff --git a/src/read-only-handler.ts b/src/read-only-handler.ts
@@ -6,18 +6,17 @@ import {
     getOwnPropertyDescriptor,
     getOwnPropertyNames,
     getOwnPropertySymbols,
+    hasOwnProperty,
 } from './shared';
 
 import {
     ReactiveMembrane,
     ReactiveMembraneShadowTarget,
+    wrapDescriptor,
 } from './reactive-membrane';
 
-function wrapDescriptor(membrane: ReactiveMembrane, descriptor: PropertyDescriptor): PropertyDescriptor {
-    if ('value' in descriptor) {
-        descriptor.value = membrane.valueIsObservable(descriptor.value) ? membrane.getReadOnlyProxy(descriptor.value) : descriptor.value;
-    }
-    return descriptor;
+function wrapReadOnlyValue(membrane: ReactiveMembrane, value: any): any {
+    return membrane.valueIsObservable(value) ? membrane.getReadOnlyProxy(value) : value;
 }
 
 export class ReadOnlyHandler {
@@ -85,16 +84,25 @@ export class ReadOnlyHandler {
             return desc;
         }
         const shadowDescriptor = getOwnPropertyDescriptor(shadowTarget, key);
-        if (!desc.configurable && !shadowDescriptor) {
+        if (!isUndefined(shadowDescriptor)) {
+            return shadowDescriptor;
+        }
+        // Note: by accessing the descriptor, the key is marked as observed
+        // but access to the value or getter (if available) cannot be observed,
+        // just like regular methods, in which case we just do nothing.
+        desc = wrapDescriptor(membrane, desc, wrapReadOnlyValue);
+        if (hasOwnProperty.call(desc, 'set')) {
+            desc.set = undefined; // readOnly membrane does not allow setters
+        }
+        if (!desc.configurable) {
             // If descriptor from original target is not configurable,
             // We must copy the wrapped descriptor over to the shadow target.
             // Otherwise, proxy will throw an invariant error.
             // This is our last chance to lock the value.
             // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/handler/getOwnPropertyDescriptor#Invariants
-            desc = wrapDescriptor(membrane, desc);
             ObjectDefineProperty(shadowTarget, key, desc);
         }
-        return shadowDescriptor || desc;
+        return desc;
     }
     preventExtensions(shadowTarget: ReactiveMembraneShadowTarget): boolean {
         if (process.env.NODE_ENV !== 'production') {

diff --git a/src/shared.ts b/src/shared.ts
@@ -10,6 +10,7 @@ const {
     getOwnPropertyNames,
     getOwnPropertySymbols,
     preventExtensions,
+    hasOwnProperty,
 } = Object;
 
 const {
@@ -32,6 +33,7 @@ export {
     getOwnPropertyNames,
     getOwnPropertySymbols,
     preventExtensions,
+    hasOwnProperty,
 };
 
 const OtS = {}.toString;

diff --git a/test/reactive-handler.spec.ts b/test/reactive-handler.spec.ts
@@ -594,7 +594,7 @@ describe('ReactiveHandler', () => {
         expect(accessSpy).toHaveBeenLastCalledWith(obj.foo, 'bar');
     });
 
-    describe.skip('issue#20 - getOwnPropertyDescriptor', () => {
+    describe('issue #20 - getOwnPropertyDescriptor', () => {
         it('should return reactive proxy when property value accessed via accessor descriptor', () => {
             const target = new ReactiveMembrane();
             const todos = {};
@@ -631,5 +631,52 @@ describe('ReactiveHandler', () => {
             const { value } = desc;
             expect(value).toBe(expected);
         });
-    })
+        it('should allow set invocation via descriptor', () => {
+            const target = new ReactiveMembrane();
+            const todos = {};
+            let value = 0;
+            const newValue = {};
+            Object.defineProperty(todos, 'entry', {
+                get() {
+                    return value;
+                },
+                set(v) {
+                    value = v;
+                },
+                configurable: true
+            });
+            const proxy = target.getProxy(todos);
+            const desc = Object.getOwnPropertyDescriptor(proxy, 'entry');
+            const { set, get } = desc;
+            set.call(proxy, newValue);
+            expect(todos.entry).toEqual(newValue);
+            expect(proxy.entry).toEqual(get.call(proxy));
+        });
+        it('should be reactive when descriptor is accessed', () => {
+            let observedTarget;
+            let observedKey;
+            const target = new ReactiveMembrane({
+                valueObserved(o, key) {
+                    observedTarget = o;
+                    observedKey = key;
+                }
+            });
+            const todos = {};
+            const observable = {};
+            Object.defineProperty(todos, 'foo', {
+                get() {
+                    return observable;
+                },
+                configurable: true
+            });
+            const expected = target.getProxy(observable);
+
+            const proxy = target.getProxy(todos);
+            expect(proxy.foo).toBe(expected);
+
+            const desc = Object.getOwnPropertyDescriptor(proxy, 'foo');
+            expect(observedKey).toBe('foo');
+            expect(observedTarget).toBe(todos);
+        });
+    });
 });
diff --git a/test/read-only-handler.spec.ts b/test/read-only-handler.spec.ts
@@ -271,7 +271,7 @@ describe('ReadOnlyHandler', () => {
             doNothing(readOnly.foo);
         }).not.toThrow();
     });
-    describe.skip('issue#20 - getOwnPropertyDescriptor', () => {
+    describe('issue #20 - getOwnPropertyDescriptor', () => {
         it('readonly proxy prevents mutation when value accessed via accessor descriptor', () => {
             const target = new ReactiveMembrane();
             const todos = {};
@@ -285,10 +285,10 @@ describe('ReadOnlyHandler', () => {
             const proxy = target.getReadOnlyProxy(todos);
             const desc = Object.getOwnPropertyDescriptor(proxy, 'entry');
             const { get } = desc;
-            expect(() => { 
+            expect(() => {
                 get().foo = '';
             }).toThrow();
-            expect(todos['entry'].foo).toEqual('bar');
+            expect(todos.entry.foo).toEqual('bar');
         });
         it('readonly proxy prevents mutation when value accessed via data descriptor', () => {
             const target = new ReactiveMembrane();
@@ -304,7 +304,54 @@ describe('ReadOnlyHandler', () => {
             expect( () => {
                 value.foo = '';
             }).toThrow();
-            expect(todos['entry'].foo).toEqual('bar');
+            expect(todos.entry.foo).toEqual('bar');
+        });
+        it('readonly proxy prevents access to any setter in descriptor', () => {
+            const target = new ReactiveMembrane();
+            const todos = {};
+            let value = 0;
+            Object.defineProperty(todos, 'entry', {
+                get() {
+                    return value;
+                },
+                set(v) {
+                    value = v;
+                },
+                configurable: true
+            });
+            const proxy = target.getReadOnlyProxy(todos);
+            const desc = Object.getOwnPropertyDescriptor(proxy, 'entry');
+            const { set, get } = desc;
+            expect(() => {
+                set.call(proxy, 1);
+            }).toThrow();
+            expect(get.call(proxy)).toEqual(0);
+        });
+        it('should be reactive when descriptor is accessed', () => {
+            let observedTarget;
+            let observedKey;
+            const target = new ReactiveMembrane({
+                valueObserved(o, key) {
+                    observedTarget = o;
+                    observedKey = key;
+                }
+            });
+            const todos = {};
+            const observable = {};
+            Object.defineProperty(todos, 'foo', {
+                get() {
+                    return observable;
+                },
+                configurable: true
+            });
+            const expected = target.getProxy(observable);
+
+            const proxy = target.getProxy(todos);
+            expect(proxy.foo).toBe(expected);
+
+            const desc = Object.getOwnPropertyDescriptor(proxy, 'foo');
+            expect(observedKey).toBe('foo');
+            expect(observedTarget).toBe(todos);
         });
     });
     it('should throw when attempting to change the prototype', function() {