fix(engine): issue #858 to enable the ability to have setters reactive #1038
Conversation
This comment has been minimized.
This comment has been minimized.
| @@ -79,23 +80,20 @@ export function linkComponent(vm: VM) { | |||
| } | |||
| } | |||
|
|
|||
| export function clearReactiveListeners(vm: VM) { | |||
There was a problem hiding this comment.
moved this logic into watcher.
| const set = deps[i]; | ||
| const pos = ArrayIndexOf.call(deps[i], vm); | ||
| function getObservingTemplateRecord(vm: VM): ObservingRecord { | ||
| return { |
There was a problem hiding this comment.
this is the record created per component to observe things accessed from the template, with the corresponding notification mechanism (notify member) to be invoked when reactive objects observed by this record are updated.
| @@ -104,7 +102,11 @@ export function renderComponent(vm: VM): VNodes { | |||
| assert.invariant(vm.isDirty, `${vm} is not dirty.`); | |||
| } | |||
|
|
|||
| clearReactiveListeners(vm); | |||
| if (!isNull(vm.otr)) { | |||
There was a problem hiding this comment.
we cache the observing template record in the VM since it is a 1-1 to the component instance, and we create it only once (the first time the component is rendered)
There was a problem hiding this comment.
is it possible for vm to not be defined here?
There was a problem hiding this comment.
Is there a consequence of caching here given that users can inject elements manually?
There was a problem hiding this comment.
that's not longer the case, now VM will always have elm and component, UninitializedVM on the other hand, doesn't
| } | ||
|
|
||
| function getObservingAccessorRecord(vm: VM, set: (v: any) => void): ObservingAccessorRecord { | ||
| return { |
There was a problem hiding this comment.
this returns the record created per instance per public setter that can be used to track reactive objects used by the original setter invocation, so when those reactive objects are updated, the notify() method is invoked to re-execute the original setter.
| const currentObservingRecordInception = getCurrentObservingRecord(); | ||
| let oRecord = vm.oar[key as any]; | ||
| if (!isUndefined(oRecord)) { | ||
| resetObservingRecord(oRecord); |
There was a problem hiding this comment.
every time the setter is invoked from outside, we clear up the record, and execute the original setter to add new listeners to this record in case things changes from one invocation to another (if conditions and such)
| @@ -87,6 +88,8 @@ export function invokeComponentRenderMethod(vm: VM): VNodes { | |||
| establishContext(context); | |||
| const isRenderingInception = isRendering; | |||
| const vmBeingRenderedInception = vmBeingRendered; | |||
| const currentObservingRecordInception = getCurrentObservingRecord(); | |||
| setCurrentObservingRecord(vm.otr); | |||
There was a problem hiding this comment.
we were relying on vmBeingRendered in the pass but now we need the actual record... that's why we added this extra thing
| export function notifyMutation(target: object, key: PropertyKey) { | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| assert.invariant(!isRendering, `Mutating property ${toString(key)} of ${toString(target)} is not allowed during the rendering life-cycle of ${vmBeingRendered}.`); | ||
| /** |
There was a problem hiding this comment.
this file has the majority of the code changes... we moved all things related to watching and new records into this file...
There was a problem hiding this comment.
More documentation explaining the different data structure and how they interact together, with maybe a schema, would greatly help here since this piece takes of the code requires a certain amount of time to wrap my head around.
| if (observingRecords) { | ||
| for (let i = 0, len = observingRecords.length; i < len; i += 1) { | ||
| const oRecord = observingRecords[i]; | ||
| oRecord.notify(); |
There was a problem hiding this comment.
in the pass, because we had only one type of observing records (it was the VM itself), we were carry on the actual logic... but now we have abstracted that out, and we just need to call the notify() method of the record, and each record can react in any way they want.
| * notified. | ||
| */ | ||
| export function resetObservingRecord(oRecord: ObservingRecord) { | ||
| const { listeners } = oRecord; |
There was a problem hiding this comment.
when a observing record needs to be reset, it means all reactive objects bound to that record for observability needs to be cleared up so when they change, this record is not notified anymore.
| * - Accessor Invocation Phase | ||
| */ | ||
| export interface ObservingRecord { | ||
| readonly vm: VM; |
There was a problem hiding this comment.
This field vm on the ObservingRecord is not being used anywhere(or VSCode is lying about its references :D ). Can be removed?
There was a problem hiding this comment.
I think you're right! I was using it before until I realized that the one from the closure is sufficient.
| } | ||
| deps.length = 0; | ||
| } | ||
| if (!vm.isDirty) { |
There was a problem hiding this comment.
Maybe you meant to read the vm from the ObservingRecord instead of creating a closure on the function param.
There was a problem hiding this comment.
yeah, we always have that dilema, closure or local ref... will try both... whatever is faster
There was a problem hiding this comment.
Did you made any performance measurement for this?
There was a problem hiding this comment.
we have done it in the past I think @pmdartus but it is tricky... honestly, I don't remember the conclusion.
| if (process.env.NODE_ENV !== 'production') { | ||
| assert.invariant(pos > -1, `when clearing up deps, the vm must be part of the collection.`); | ||
| assert.invariant(!isRendering, `Mutating property is not allowed during the rendering life-cycle of ${vmBeingRendered}.`); |
There was a problem hiding this comment.
would it be more clear to the developers if we used an element name or a component in this message?
| @@ -104,7 +102,11 @@ export function renderComponent(vm: VM): VNodes { | |||
| assert.invariant(vm.isDirty, `${vm} is not dirty.`); | |||
| } | |||
|
|
|||
| clearReactiveListeners(vm); | |||
| if (!isNull(vm.otr)) { | |||
There was a problem hiding this comment.
is it possible for vm to not be defined here?
| @@ -104,7 +102,11 @@ export function renderComponent(vm: VM): VNodes { | |||
| assert.invariant(vm.isDirty, `${vm} is not dirty.`); | |||
| } | |||
|
|
|||
| clearReactiveListeners(vm); | |||
| if (!isNull(vm.otr)) { | |||
There was a problem hiding this comment.
Is there a consequence of caching here given that users can inject elements manually?
| @@ -102,6 +103,33 @@ function createPublicPropertyDescriptor(proto: ComponentConstructor, key: Proper | |||
| }; | |||
| } | |||
|
|
|||
| interface ObservingAccessorRecord extends ObservingRecord { | |||
There was a problem hiding this comment.
Would something like ObservableRecord be more readable instead of Observing? To me, the name reads like an action in its current form
There was a problem hiding this comment.
this is a record that represents a accessor that is invoked during the observing phase, we already have a ObservableRecord for the actual object and the corresponding property from that obj.
| @@ -102,6 +103,33 @@ function createPublicPropertyDescriptor(proto: ComponentConstructor, key: Proper | |||
| }; | |||
| } | |||
|
|
|||
| interface ObservingAccessorRecord extends ObservingRecord { | |||
| value?: any; | |||
There was a problem hiding this comment.
how can we have an observed record without a value? Wouldn't we at least have undefined?
There was a problem hiding this comment.
when the record is created the first time, it is right before the value is handled and wrapped with a proxy, so, at this point we don't have it... additionally, it could be undefined, I think I can remove the ? and set it to undefined.
| @@ -154,9 +158,18 @@ export function removeVM(vm: VM) { | |||
| return; // already removed | |||
| } | |||
| removeInsertionIndex(vm); | |||
| // just in case it comes back, with this we guarantee re-rendering it | |||
| const { oar, otr } = vm; | |||
| // Just in case it comes back, with this we guarantee re-rendering it | |||
There was a problem hiding this comment.
does this mean the removed vm can be re-added? In which scenario can this occur?
There was a problem hiding this comment.
root elements can be inserted and removed, and inserted back
| resetObservingRecord(otr as ObservingRecord); | ||
| } | ||
| // Making sure that any observing accessor record will not trigger the setter to be reinvoked | ||
| for (const key in oar) { |
There was a problem hiding this comment.
why not just reset the entire oar record?
There was a problem hiding this comment.
this is a collection of records, each of then has their own cleaning mechanism to avoid notifications when the component is disconnected.
| // used to track down all object-key pairs that makes this vm reactive | ||
| deps: [], | ||
| otr: null, | ||
| oar: create(null), |
There was a problem hiding this comment.
why do assigned values differ between oar and otr?
There was a problem hiding this comment.
otr = observing template record
oar: observing accessor records
template you have only one per instance, accessors you have multiple accessors per instance.
| } | ||
|
|
||
| /** | ||
| * An Observed Membre Property Record represents the list of all Observing Records, |
| return currentObservingRecord; | ||
| } | ||
|
|
||
| export function notifyMutation(target: object, key: PropertyKey) { | ||
| const reactiveRecord = TargetToReactiveRecordMap.get(target); |
There was a problem hiding this comment.
in getReactiveRecord function, we set the value of the target to null. Wouldn't line 59 cause an error?
There was a problem hiding this comment.
we never set it to null, we set it to a dictionary: Object.create(null)
caridy
force-pushed
the
caridy/issue-858/reactive-setter
branch
from
b93b0d8
to
8744689
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
caridy
force-pushed
the
caridy/issue-858/reactive-setter
branch
2 times, most recently
from
d913363
to
793e88c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } | ||
| deps.length = 0; | ||
| } | ||
| if (!vm.isDirty) { |
There was a problem hiding this comment.
This is part of the critical path you should probably use isFalse instead of !
| } | ||
| deps.length = 0; | ||
| } | ||
| if (!vm.isDirty) { |
There was a problem hiding this comment.
Did you made any performance measurement for this?
| } | ||
| }, | ||
| debouncing: false, | ||
| } as ObservingAccessorRecord; |
There was a problem hiding this comment.
You should not need to cast here, why not adding the vm field to ObservingAccessorRecord.
There was a problem hiding this comment.
we had that in previous versions... but removed in favor of the always present closure.
| vm, | ||
| value: undefined, | ||
| listeners: [], | ||
| notify(this: ObservingAccessorRecord) { |
There was a problem hiding this comment.
Why do we need a closure here and not store the set property on the ObservingAccessorRecord itself?
There was a problem hiding this comment.
yes, still the question from above is lingering... is it really faster that way?
There was a problem hiding this comment.
I think so, the creation of closure store an instance of the function + its context. We need to experiement here, but I think it would give allow us to mitigate the regression we currently have compared to master.
| @@ -106,7 +103,11 @@ export function renderComponent(vm: VM): VNodes { | |||
| assert.invariant(vm.isDirty, `${vm} is not dirty.`); | |||
| } | |||
|
|
|||
| clearReactiveListeners(vm); | |||
| if (!isNull(vm.otr)) { | |||
There was a problem hiding this comment.
Why not creating the otr at the VM creation time, we know for fact that the renderComponent will be invoked in the future. This would avoid an extra branching here.
There was a problem hiding this comment.
Yeah, in the past it was like that... I can do that.
| return reactiveRecord; | ||
| } | ||
|
|
||
| export let currentObservingRecord: ObservingRecord | null = null; |
There was a problem hiding this comment.
currentObservingRecord should not be exported if there is a getter and a setter.
There was a problem hiding this comment.
can you elaborate more?
There was a problem hiding this comment.
currentObservingRecord is only consumed locally, I don't think it should be exported outside the module.
| export function notifyMutation(target: object, key: PropertyKey) { | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| assert.invariant(!isRendering, `Mutating property ${toString(key)} of ${toString(target)} is not allowed during the rendering life-cycle of ${vmBeingRendered}.`); | ||
| /** |
There was a problem hiding this comment.
More documentation explaining the different data structure and how they interact together, with maybe a schema, would greatly help here since this piece takes of the code requires a certain amount of time to wrap my head around.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Benchmark resultsBase commit: |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
caridy
force-pushed
the
caridy/issue-858/reactive-setter
branch
from
5952e05
to
6ef253d
Compare
This comment has been minimized.
This comment has been minimized.
caridy
force-pushed
the
caridy/issue-858/reactive-setter
branch
from
6ef253d
to
32694a3
Compare
|
Thanks for the contribution! Before we can merge this, we need @caridy to sign the Salesforce.com Contributor License Agreement. |
This comment has been minimized.
This comment has been minimized.
fix(reactive-service): rebasing master fix(reactive-service): delinting fix(reactive-service): adding units fix(reactive-service): pkg cleanup fix(engine): perf optimizations on VM declaration
caridy
force-pushed
the
caridy/issue-858/reactive-setter
branch
from
32694a3
to
60b4852
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
| valueMutated(o, 'x'); | ||
| expect(changed).toBe(false); | ||
| }); | ||
| it('should support observing and mutating the same value multiple times', () => { |
There was a problem hiding this comment.
@caridy this (maybe other test?!) is throwing in the ci with error:
@lwc/engine: PASS lwc-engine src/libs/mutation-tracker/__tests__/index.spec.ts
@lwc/engine: (node:308) UnhandledPromiseRejectionWarning: Error: expect(received).toBe(expected) // Object.is equality
@lwc/engine: Expected: 2
@lwc/engine: Received: 1
@lwc/engine: (node:308) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
@lwc/engine: (node:308) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
does not seems to be doing anything async, or it does?
There was a problem hiding this comment.
this is a weird thing that I have been for a while... I don't know what is it, but it is not from this PR, I Can confirm that.
Benchmark resultsBase commit: lwc-engine-benchmark
|
| import Parent from 'x/parent'; | ||
|
|
||
| // TODO: issue #858 to enable reactive setters | ||
| xdescribe('Reactivity for setters', () => { |
There was a problem hiding this comment.
Why is this disabled? this should make them pass right?
There was a problem hiding this comment.
also, should we remove the TODO on the line above?
There was a problem hiding this comment.
This PR is not going to be squashed, it will have 3 commits, the second commit is the revert of the reactive setter until we get the feature flagging system that @ekashida is working on, once that lands, we enable this test, revert that second PR and enable the reactie setters :), this PR enables that to happen, doesn't change any functionality in the engine
fix(engine): extracting out reactive-service into pkg mutation-tracker fix(engine): extracting out reactive-service into libs/mutation-tracker fix(engine): extracting out reactive-service into libs/mutation-tracker fix(engine): extracting out reactive-service into libs/mutation-tracker
caridy
force-pushed
the
caridy/issue-858/reactive-setter
branch
from
db6fba6
to
5e0fa97
Compare
caridy
changed the title
caridy
changed the title
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Details
This PR is a refactor to organize the reactive mechanism so it can be used in other areas other than template rendering.
Does this PR introduce a breaking change?