refactor(engine): avoiding proxies of proxies in membranes #183
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
caridy
commented
Mar 28, 2018
| return unwrapped; | ||
| } | ||
| // piercing membrane is not that important, it goes second | ||
| unwrapped = (value && getKey(value, TargetSlot)) || value; |
There was a problem hiding this comment.
simplification of the previous unwrapping method to work faster, no more for-loop, just plain and simple sequencial unwrapping until finding a match.
also I have reordered the sequence to have the most common case first.
Benchmark comparisonBase commit:
|
caridy
commented
Mar 29, 2018
| import { ReactiveMembrane, unwrap as observableUnwrap } from "observable-membrane"; | ||
| import { observeMutation, notifyMutation } from "./watcher"; | ||
|
|
||
| function format(value: any) { |
There was a problem hiding this comment.
code moved from reactive.ts to avoid circular deps.
caridy
commented
Mar 29, 2018
| if (process.env.NODE_ENV !== 'production') { | ||
| // For now, if we determine that value is a piercing membrane | ||
| // we want to throw a big error. | ||
| if (replicaUnwrap(value) !== value) { |
There was a problem hiding this comment.
this is one of the new tricks. This covers the case where:
- attempting to access a piercing membrane object (we call them replicas) via a reactive membrane.
For now, this only affects dev-mode, and it throws a big error. We can bikeshed on the text of the error!
There was a problem hiding this comment.
Why throw? Why not just return the unwrapped version and let observable membrane handle it?
caridy
commented
Mar 29, 2018
| return value; | ||
| } | ||
|
|
||
| export const reactiveMembrane = new ReactiveMembrane(format, { |
There was a problem hiding this comment.
code from reactive.ts! intact!
caridy
commented
Mar 29, 2018
| }); | ||
|
|
||
| // TODO: REMOVE THIS https://github.com/salesforce/lwc/issues/129 | ||
| export function dangerousObjectMutation(obj: any): any { |
There was a problem hiding this comment.
code from reactive.ts! intact!
caridy
commented
Mar 29, 2018
| if (!isReplicable(replicaRawValue)) { | ||
| return replicaRawValue; | ||
| } | ||
| const reactiveRawValue = observableUnwrap(replicaRawValue); |
There was a problem hiding this comment.
this is the second trick! which guarantees that:
- if you ever access a reactive object via a piercing membrane, what you get is the readonly version of the reactive object.
this.root.querySelector('x-foo').publicPropX; // what you get here is the readOnly version of `publicPropX` from `x-foo` instance.
caridy
commented
Mar 29, 2018
| @@ -1,39 +0,0 @@ | |||
| import assert from './assert'; | |||
There was a problem hiding this comment.
folding this file into membrane.ts
caridy
commented
Mar 29, 2018
| @@ -58,3 +55,7 @@ export function observeMutation(target: object, key: PropertyKey) { | |||
| ArrayPush.call(vm.deps, value); | |||
| } | |||
| } | |||
|
|
|||
| import { scheduleRehydration, VM } from "./vm"; | |||
There was a problem hiding this comment.
moving these imports down helps with the circular deps problem
Benchmark comparisonBase commit:
|
byao
reviewed
Mar 29, 2018
| @@ -10,7 +11,7 @@ describe('unwrap', () => { | |||
|
|
|||
| it('should unwrap observable membrane object correctly', () => { | |||
| const obj = {}; | |||
| const proxy = membrane.getProxy(obj); | |||
| const proxy = reactiveMembrane.getProxy(obj); | |||
| expect(unwrap(proxy)).toBe(obj); | |||
There was a problem hiding this comment.
can we add test to unwrap and verify not proxy? what check is toBe ?
| if (process.env.NODE_ENV !== 'production') { | ||
| // For now, if we determine that value is a piercing membrane | ||
| // we want to throw a big error. | ||
| if (replicaUnwrap(value) !== value) { |
There was a problem hiding this comment.
Why throw? Why not just return the unwrapped version and let observable membrane handle it?
| const getKey = ProxyGetKey ? ProxyGetKey : (o: any, key: PropertyKey): any => o[key]; | ||
|
|
||
| export function replicaUnwrap(value: any): any { | ||
| // observable membrane goes first because it is in the critical path |
There was a problem hiding this comment.
I don't think this comment makes sense here
davidturissini
suggested changes
Mar 29, 2018
davidturissini
approved these changes
Mar 29, 2018
davidturissini
pushed a commit
that referenced
this pull request
Mar 29, 2018
* refactor(engine): avoid rewrapping of proxies * refactor(engine): prevent leaking double proxies via events or querySelector
davidturissini
added a commit
that referenced
this pull request
Mar 29, 2018
caridy
added a commit
that referenced
this pull request
May 2, 2018
* refactor(engine): avoid rewrapping of proxies * refactor(engine): prevent leaking double proxies via events or querySelector fix(aom): consolidating the aria dom property names (#192) wip: Remove slotting refactor(engine): conditinally use of native shadowRoot fix(engine): compat should also work with artificial shadow refactor(engine): adding slotting refactor(engine): adding slotting refactor(engine): conditionally piercing and default behaviors refactor(engine): minimizing the polyfilling on shadow-root to only AOM refactor(engine): implementing Pierre's suggestion to avoid patching shadow root prototype
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
This PR prevents:
Does this PR introduce a breaking change?