fix: membrane consolidation to work with env virtualization by caridy · Pull Request #49 · salesforce/observable-membrane

caridy · 2020-06-02T03:56:16Z

never using the shadowTarget to read from it, it is only there for the invariants
fix unwrapping of accessor descriptors
moving shared code into a new BaseProxyHandler that shared 80% of the logic between the two handlers (this file actually have all the shadowTarget logic from membrane as well)

src/reactive-handler.ts

caridy · 2020-06-02T03:58:34Z

src/reactive-handler.ts

        /* No op */
    }
-    construct(target: ReactiveMembraneShadowTarget, argArray: any, newTarget?: any): any {
+    construct(shadowTarget: ReactiveMembraneShadowTarget, argArray: any, newTarget?: any): any {


just to keep consistency

src/reactive-handler.ts

caridy · 2020-06-02T03:59:20Z

src/reactive-handler.ts

-        lockShadowTarget(membrane, shadowTarget, originalTarget);
-        preventExtensions(originalTarget);
-        return true;
+        return preventExtensionsMembraneTrap.call(this, shadowTarget);


well, we are not doing much optimizations around this... copying again into the shadow target should not be a problem, it is just overhead, but the reality is that must of these proxies will never be non-configurable.

src/reactive-handler.ts

src/reactive-membrane.ts

caridy · 2020-06-02T04:01:19Z

src/reactive-membrane.ts


-export function wrapDescriptor(membrane: ReactiveMembrane, descriptor: PropertyDescriptor, getValue: (membrane: ReactiveMembrane, originalValue: any) => any): PropertyDescriptor {
-    const { set, get } = descriptor;
+const reserveGetterMap = new WeakMap<() => any, () => any>();


the reverse maps are used to unwrap accessor descriptors back into their original values, we were leaking membrane generated accessor wrappers into the original target when redefining properties :(

need tests

caridy · 2020-06-02T04:02:11Z

src/reactive-membrane.ts

+    } else {
+        const { set: originalSet, get: originalGet } = descriptor;
+        if (!isUndefined(originalGet)) {
+            const get = handler.wrapGetter(originalGet);


not longer creating a new function every time... instead using the weakmap for caching and identity preservation

caridy · 2020-06-02T04:02:17Z

src/reactive-membrane.ts

+            descriptor.get = get;
+        }
+        if (!isUndefined(originalSet)) {
+            const set = handler.wrapSetter(originalSet);


caridy · 2020-06-02T04:03:04Z

src/reactive-membrane.ts

+        // dealing with a data descriptor
+        descriptor.value = unwrap(descriptor.value);
    } else {
+        const { set, get } = descriptor;


the previous unwrap was not even unwrapping accessors :(

caridy · 2020-06-02T04:03:26Z

src/reactive-membrane.ts

    return descriptor;
 }

+export function copyDescriptorIntoShadowTarget(


abstraction

caridy · 2020-06-02T04:05:44Z

src/reactive-membrane.ts

+        return desc;
+    }
+
+    if (desc.configurable === false) {


this is probably the block with the bigger changes... in the previous version of this, from each handler, the descriptor from the shadowTarget was read, and used when possible... that on itself poses many problems, but the most important one is the following:

you might get an old value from an old installed non-configurable descriptor that is writable, e.g.: array.length

need tests

caridy · 2020-06-02T04:06:17Z

src/reactive-membrane.ts

+    shadowTarget: ReactiveMembraneShadowTarget
+): boolean {
+    const { originalTarget } = this;
+    if (isExtensible(shadowTarget)) {


improve it to support proxy as targets, something that wasn't really work before.

caridy · 2020-06-02T04:06:52Z

src/read-only-handler.ts

-export class ReadOnlyHandler {
-    private originalTarget: any;
-    private membrane: ReactiveMembrane;
+export class ReadOnlyHandler implements MembraneProxyHandler {


this is pretty much the same as the previous file... same comments apply here.

caridy · 2020-06-02T04:09:20Z

src/read-only-handler.ts

+            return setterMap.get(originalSet) as (v: any) => void;
+        }
+        const handler = this;
+        function set(this: any, v: any) {


@ravijayaramappa this is probably the biggest difference. In the old code, the setter on read-only handlers was always undefined, but getting it to undefined will imply that we will not be able to unwrap it to the original implementation anymore. Instead, we are producing a setter, but it throws when invoked. That seems to be equivalent in behavior, but it is an observable difference. Also, in production, it doesn't throw, it doesn't do anything.

src/reactive-handler.ts

src/reactive-membrane.ts

+    // 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.
+    return wrapDescriptor(this, desc);


src/reactive-membrane.ts

src/reactive-handler.ts

-        lockShadowTarget(membrane, shadowTarget, originalTarget);
-        preventExtensions(originalTarget);
-        return true;
+        return preventExtensionsMembraneTrap.call(this, shadowTarget);


Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>

src/reactive-handler.ts

apapko

Few question and minor recommendations.

src/base-handler.ts

src/reactive-handler.ts

src/read-only-handler.ts

src/base-handler.ts

src/reactive-handler.ts

src/read-only-handler.ts

src/reactive-handler.ts

Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com>

test/reactive-handler.spec.ts

test/read-only-handler.spec.ts

Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>

fix: membrane consolidation to work with env virtualization

79f58ad

caridy requested review from jdalton and ravijayaramappa June 2, 2020 03:56