fix: refactor reactive membrane to use weakmap instead of targetslot #34
Conversation
ravijayaramappa
changed the title
ravijayaramappa
changed the title
There was a problem hiding this comment.
LGTM. As for the suggestion from @davidturissini to unwrap to the distorted value instead of the original value, we found a reason for the change to make sense, which is, once you unwrap during the getProxy or getReadOnlyProxy, you then call distortion callback, which should always get the original value rather than the already distorted value, otherwise it is really easy to trip down in the implementation of the valueDistortion routine.
| const original = getTarget(item); | ||
| if (original) { | ||
| const original = unwrap(item); | ||
| if (original !== item) { |
There was a problem hiding this comment.
Is this to handle a proxy within a proxy?
There was a problem hiding this comment.
Yes. As per my understanding, getTarget(item) is a check to see if item is proxy. If it is, then it recursively extracts that proxy. The new logic is "Can this value be unwrapped? If yes, then its a proxy, recursively extract."
| @@ -63,9 +65,6 @@ export class ReactiveProxyHandler { | |||
| } | |||
| get(shadowTarget: ReactiveMembraneShadowTarget, key: PropertyKey): any { | |||
| const { originalTarget, membrane } = this; | |||
| if (key === TargetSlot) { | |||
| return originalTarget; | |||
| ObjectDefineProperty(this, 'readOnly', { value: proxy }); | ||
| return proxy; | ||
| } | ||
| } as ReactiveState; | ||
|
|
||
| objectGraph.set(value, reactiveState); | ||
| objectGraph.set(distortedValue, reactiveState); |
There was a problem hiding this comment.
Why do we want to use distorderValue here instead of value?
There was a problem hiding this comment.
Because of two reasons
- The handler's behavior is based on the distorted value. So the object graph should have a 1:1 relationship with the value that it is encapsulating.
- Handles both deterministic and non-deterministic distortion
- If distortion is deterministic, then there is a 1:1 mapping between value <-> distortedValue. So we can use either, it makes no difference.
- If distortion is non-deterministic,
- And, the object graph is keyed by
value, then we will always produce the same ReactiveState for a given value. We are removing the non-deterministic nature of the distortion. Which may be or may not be desirable. - On the other hand, if the object graph is keyed by
distortedValue, then we will produce a ReactiveState for every unique distortion.
- And, the object graph is keyed by
This PR is addressing two issues
A reactive object looses its reactivity, when the reactive membrane is requested for a read-only version of the object. The caveat here is that the reactive proxy is wrapped in an external proxy(for example, the locker proxy).
The root cause here is two fold:
a. When requested for a read-only version of a reactive object, the reactive membrane tries to identify the reactive state of the original object by unwrapping the reactive proxy.
b. The unwrap mechanism uses a symbol lookup which is handled by the proxy get trap. If the reactive proxy is wrapped in an external proxy, the unwrap mechanism is not supposed to work because the reactive membrane should not be able to demarshal(unwrap) an external proxy.
Currently, the symbol lookup resolves and returns the original value to the external proxy and the external proxy can transform the original value. This transformed value is returned to the reactive membrane and it in turn wraps that transformed value in a read only proxy.
The unwrapProxy() api on the reactive membrane instance is returning the distorted value. It is expected to return the original value that is represented by the reactive/read-only proxy.
Issue - W-5925656