-
Notifications
You must be signed in to change notification settings - Fork 27
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.
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: membrane consolidation to work with env virtualization #49
Conversation
caridy
commented
Jun 2, 2020
•
edited
Loading
edited
- 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
Outdated
@@ -94,7 +88,7 @@ export class ReactiveProxyHandler { | |||
apply(shadowTarget: ReactiveMembraneShadowTarget, thisArg: any, argArray: any[]) { | |||
/* No op */ | |||
} | |||
construct(target: ReactiveMembraneShadowTarget, argArray: any, newTarget?: any): any { | |||
construct(shadowTarget: ReactiveMembraneShadowTarget, argArray: any, newTarget?: any): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to keep consistency
src/reactive-handler.ts
Outdated
lockShadowTarget(membrane, shadowTarget, originalTarget); | ||
preventExtensions(originalTarget); | ||
return true; | ||
return preventExtensionsMembraneTrap.call(this, shadowTarget); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abstracted
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-membrane.ts
Outdated
@@ -76,31 +90,150 @@ 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; | |||
const reserveGetterMap = new WeakMap<() => any, () => any>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/reactive-membrane.ts
Outdated
} else { | ||
const { set: originalSet, get: originalGet } = descriptor; | ||
if (!isUndefined(originalGet)) { | ||
const get = handler.wrapGetter(originalGet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not longer creating a new function every time... instead using the weakmap for caching and identity preservation
src/reactive-membrane.ts
Outdated
descriptor.get = get; | ||
} | ||
if (!isUndefined(originalSet)) { | ||
const set = handler.wrapSetter(originalSet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same...
src/reactive-membrane.ts
Outdated
} else { | ||
const { set, get } = descriptor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the previous unwrap was not even unwrapping accessors :(
src/reactive-membrane.ts
Outdated
} | ||
} | ||
return descriptor; | ||
} | ||
|
||
export function copyDescriptorIntoShadowTarget( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abstraction
src/reactive-membrane.ts
Outdated
return desc; | ||
} | ||
|
||
if (desc.configurable === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/reactive-membrane.ts
Outdated
shadowTarget: ReactiveMembraneShadowTarget | ||
): boolean { | ||
const { originalTarget } = this; | ||
if (isExtensible(shadowTarget)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
improve it to support proxy as targets, something that wasn't really work before.
src/read-only-handler.ts
Outdated
export class ReadOnlyHandler { | ||
private originalTarget: any; | ||
private membrane: ReactiveMembrane; | ||
export class ReadOnlyHandler implements MembraneProxyHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is pretty much the same as the previous file... same comments apply here.
src/read-only-handler.ts
Outdated
return setterMap.get(originalSet) as (v: any) => void; | ||
} | ||
const handler = this; | ||
function set(this: any, v: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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-membrane.ts
Outdated
// 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); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/reactive-handler.ts
Outdated
lockShadowTarget(membrane, shadowTarget, originalTarget); | ||
preventExtensions(originalTarget); | ||
return true; | ||
return preventExtensionsMembraneTrap.call(this, shadowTarget); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few question and minor recommendations.
Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com>
Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>