feat: track decorator reform #1428
Merged
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
Benchmark resultsBase commit: lwc-engine-benchmark
|
jodarove
force-pushed
the
jodarove/track-rfc-implementation
branch
from
239c569 to
a55fdc4
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
jodarove
force-pushed
the
jodarove/track-rfc-implementation
branch
2 times, most recently
from
557c49e to
48413b4
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
reviewed
Aug 9, 2019
| @@ -89,6 +89,7 @@ export interface UninitializedVM { | |||
| cmpProps: any; | |||
| cmpSlots: SlotSet; | |||
| cmpTrack: any; | |||
| cmpFields: any; | |||
There was a problem hiding this comment.
don't add a new one... use cmpTrack instead because in another PR (wire reform), I'm renaming this to cmpFields as a single place to store such values.
caridy
reviewed
Aug 9, 2019
| @@ -231,6 +232,7 @@ export function createVM(elm: HTMLElement, Ctor: ComponentConstructor, options: | |||
| context: create(null), | |||
| cmpProps: create(null), | |||
| cmpTrack: create(null), | |||
| cmpFields: create(null), | |||
caridy
reviewed
Aug 9, 2019
| @@ -154,6 +158,7 @@ function createComponentDef( | |||
| name, | |||
| wire, | |||
| track, | |||
| fields, | |||
There was a problem hiding this comment.
this is only needed if you need to know what are the fields later on... which I don't think you need to, you just install the descriptor and forget about them. remove it.
caridy
reviewed
Aug 9, 2019
| import { defineProperty, isFalse } from '../shared/language'; | ||
|
|
||
| export function observeFields(Ctor: ComponentConstructor, fields: string[] | undefined) { | ||
| if (fields) { |
There was a problem hiding this comment.
!isUndefined(fields)
in fact I think that this condition should happens in the caller of this method... and this method, if invoked, should always have at least one field.
There was a problem hiding this comment.
this method should also receive the proto, instead of the Ctor, that's easier to reason about... you pass the obj that receive the descriptors... alternative, you call this method with the field list... and it returns a PropertyDescriptorMap that then you install on any obj, I like that better, it disconnects this from the ctor completely.
There was a problem hiding this comment.
you can rename this method to createObservableFieldsMap(fields: PropertyKey[]): PropertyDescriptorMap
caridy
reviewed
Aug 9, 2019
| !isRendering, | ||
| `${vmBeingRendered}.render() method has side effects on the state of ${vm}.${String( | ||
| key | ||
| )}` |
caridy
suggested changes
Aug 9, 2019
RFC for this change: salesforce/lwc-rfcs#4 The logic for this PR is as follows: When the compiler compiles the class, it extracts any field that is not decorated with @api, @track or @wire, and pass the metadata through the registerDecorators call. Given that in order to observe a field, we need a vm, and we need to register the fields of all classes and save it in the decoratorsMeta, we need to wait until we create the ComponentDefinition to know which classes will be used as components. For every class field we will observe in a Component, we will create a getter and a setter in the prototype of the ComponentDefinition (and the class inheritance chain until BaseLightningElement). No change is needed to the logic in the engine.
Benchmark resultsBase commit: |
jodarove
force-pushed
the
jodarove/track-rfc-implementation
branch
from
4a6a167 to
f4bb16d
Compare
jodarove
commented
Aug 9, 2019
| import { isFalse } from '../shared/language'; | ||
|
|
||
| export function createObservableFieldsDescriptorMap(fields: PropertyKey[]): PropertyDescriptorMap { | ||
| return fields.reduce((acc, field) => { |
There was a problem hiding this comment.
Use ArrayReduce from language
ekashida
requested changes
Aug 9, 2019
There was a problem hiding this comment.
Looks good overall besides the inconsistency between the terms "observable" and "observed" (the latter seems more correct to me). It does feel weird to me that we're reusing decorator plumbing to implement implicitly-tracked fields, but maybe that's because of outdated naming (e.g., DecoratorMeta should really be ComponentMeta, etc).
packages/@lwc/babel-plugin-component/src/__tests__/observable-fields.spec.js
Outdated
Show resolved
Hide resolved
packages/@lwc/babel-plugin-component/src/post-process/transform.js
Outdated
Show resolved
Hide resolved
packages/integration-karma/test/component/observed-fields/index.spec.js
Outdated
Show resolved
Hide resolved
| assert.isTrue(vm && 'cmpRoot' in vm, `${vm} is not a valid vm.`); | ||
| assert.invariant( | ||
| !isRendering, | ||
| `${vmBeingRendered}.render() method has side effects on the state of "${String( |
There was a problem hiding this comment.
Nit: Isn't the call to String(key) implicit?
There was a problem hiding this comment.
the type of key is PropertyKey which is string | number | symbol and the implicit conversion of symbol will fail at runtime, thus, the String()
Co-Authored-By: Eugene Kashida <[email protected]> Update packages/integration-karma/test/component/observed-fields/index.spec.js Co-Authored-By: Eugene Kashida <[email protected]> Update packages/@lwc/engine/src/framework/def.ts Co-Authored-By: Eugene Kashida <[email protected]> Update packages/@lwc/engine/src/framework/observable-fields.ts Co-Authored-By: Eugene Kashida <[email protected]> Update packages/@lwc/engine/src/framework/observable-fields.ts Co-Authored-By: Eugene Kashida <[email protected]> Update packages/@lwc/babel-plugin-component/src/post-process/transform.js Co-Authored-By: Eugene Kashida <[email protected]>
jodarove
force-pushed
the
jodarove/track-rfc-implementation
branch
from
e50a78b to
726f03c
Compare
caridy
reviewed
Aug 10, 2019
| } | ||
|
|
||
| export interface DecoratorMeta { | ||
| wire: WireHash | undefined; | ||
| track: TrackDef; | ||
| props: PropsDef; | ||
| methods: MethodDef; | ||
| fields?: string[]; |
There was a problem hiding this comment.
I prefer the other annotation, fields: string[] | undefined, technically it is almost the same, but the shape of the meta obj should always have the same properties, even if they are set to undefined.
caridy
approved these changes
Aug 10, 2019
|
Where are the perf numbers? |
|
@ekashida the perf numbers are only going to be posted as a comment when there is a regression, otherwise it will be accessible via the checks, the details for BEST task. |
caridy
reviewed
Aug 12, 2019
| @@ -162,6 +162,7 @@ function createComponentDef( | |||
| name, | |||
| wire, | |||
| track, | |||
| fields: undefined, | |||
There was a problem hiding this comment.
this is not needed right? we can remove this from Def completely.
There was a problem hiding this comment.
@caridy this is needed with your suggested change: fields: string[] | undefined createComponentDef receives a ComponentDef which inherit from DecoratorsMeta, and now fields can be string[] or undefined, but should be present.
This reverts commit b2cb9d5.
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
RFC for this change: salesforce/lwc-rfcs#4
The logic for this PR is as follows:
When the compiler compiles a class, it extracts any field that is not decorated with
@api,@trackor@wire, and pass the metadata through theregisterDecoratorscall.Given that in order to observe a field we need a vm, we need to register the fields of all classes and save it in the decoratorsMeta, we wait until we create the ComponentDefinition to know which classes will be used as components.
For every class field we will observe in a Component, we will create a getter and a setter in the prototype of the ComponentDefinition (and the class inheritance chain until BaseLightningElement).
No change is needed to the logic in the engine.
Does this PR introduce breaking changes?
Yes, it does introduce breaking changes.When modifying a reactive property (tracked) in the render method, it will cause rehydration during render.
This will trigger an engine invariant violation similar to the following:
Error: Invariant Violation: [object:vm undefined (500)].render() method has side effects on the state of [object:vm undefined (500)].renderCount.Upgrade path: Use the
renderedCallbackin such cases.