Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: track decorator reform #1428

Merged
merged 7 commits into from Aug 12, 2019

Conversation

@jodarove
Copy link
Contributor

jodarove commented Jul 23, 2019

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, @track or @wire, and pass the metadata through the registerDecorators call.

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 renderedCallback in such cases.

@salesforce-best-lwc-internal

This comment has been minimized.

Copy link

salesforce-best-lwc-internal bot commented Jul 23, 2019

Benchmark results

Base commit: 165ad3b | Target commit: 239c569

lwc-engine-benchmark

table-append-1k metric base(165ad3b) target(239c569) trend
benchmark-table/append/1k duration 141.10 (卤2.70 ms) 145.20 (卤4.60 ms) +4.1ms (2.9%) 馃憥
table-clear-1k metric base(165ad3b) target(239c569) trend
benchmark-table/clear/1k duration 11.35 (卤0.95 ms) 10.60 (卤0.80 ms) -0.8ms (6.6%) 馃憤
table-create-10k metric base(165ad3b) target(239c569) trend
benchmark-table/create/10k duration 844.45 (卤7.05 ms) 863.55 (卤5.35 ms) +19.1ms (2.3%) 馃憥
table-create-1k metric base(165ad3b) target(239c569) trend
benchmark-table/create/1k duration 107.60 (卤3.50 ms) 108.75 (卤2.65 ms) +1.2ms (1.1%) 馃憣
table-update-10th-1k metric base(165ad3b) target(239c569) trend
benchmark-table/update-10th/1k duration 69.00 (卤3.35 ms) 77.90 (卤3.60 ms) +8.9ms (12.9%) 馃憥
tablecmp-append-1k metric base(165ad3b) target(239c569) trend
benchmark-table-component/append/1k duration 220.80 (卤15.50 ms) 223.10 (卤7.90 ms) +2.3ms (1.0%) 馃憣
tablecmp-clear-1k metric base(165ad3b) target(239c569) trend
benchmark-table-component/clear/1k duration 5.95 (卤0.95 ms) 6.40 (卤1.15 ms) +0.5ms (7.6%) 馃憣
tablecmp-create-10k metric base(165ad3b) target(239c569) trend
benchmark-table-component/create/10k duration 1614.85 (卤9.70 ms) 1635.90 (卤20.20 ms) +21.1ms (1.3%) 馃憥
tablecmp-create-1k metric base(165ad3b) target(239c569) trend
benchmark-table-component/create/1k duration 182.75 (卤4.65 ms) 186.15 (卤5.20 ms) +3.4ms (1.9%) 馃憣
tablecmp-update-10th-1k metric base(165ad3b) target(239c569) trend
benchmark-table-component/update-10th/1k duration 65.25 (卤5.00 ms) 67.75 (卤4.05 ms) +2.5ms (3.8%) 馃憣
wc-append-1k metric base(165ad3b) target(239c569) trend
benchmark-table-wc/append/1k duration 230.45 (卤9.65 ms) 234.00 (卤8.95 ms) +3.6ms (1.5%) 馃憣
wc-clear-1k metric base(165ad3b) target(239c569) trend
benchmark-table-wc/clear/1k duration 10.60 (卤1.95 ms) 11.25 (卤2.05 ms) +0.7ms (6.1%) 馃憥
wc-create-10k metric base(165ad3b) target(239c569) trend
benchmark-table-wc/create/10k duration 1835.60 (卤16.05 ms) 1846.55 (卤10.20 ms) +11.0ms (0.6%) 馃憥
wc-create-1k metric base(165ad3b) target(239c569) trend
benchmark-table-wc/create/1k duration 221.05 (卤4.55 ms) 224.85 (卤5.65 ms) +3.8ms (1.7%) 馃憥
wc-update-10th-1k metric base(165ad3b) target(239c569) trend
benchmark-table-wc/update-10th/1k duration 66.10 (卤3.70 ms) 66.80 (卤4.30 ms) +0.7ms (1.1%) 馃憣
@jodarove jodarove force-pushed the jodarove/track-rfc-implementation branch from 239c569 to a55fdc4 Aug 5, 2019
@salesforce-best-lwc-internal

This comment has been minimized.

Copy link

salesforce-best-lwc-internal bot commented Aug 5, 2019

Benchmark results

Base commit: d5e4be3 | Target commit: a55fdc4

lwc-engine-benchmark

table-append-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table/append/1k duration 140.75 (卤3.60 ms) 139.85 (卤4.05 ms) -0.9ms (0.6%) 馃憣
table-clear-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table/clear/1k duration 10.50 (卤1.20 ms) 10.30 (卤0.80 ms) -0.2ms (1.9%) 馃憣
table-create-10k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table/create/10k duration 849.85 (卤4.75 ms) 857.45 (卤4.70 ms) +7.6ms (0.9%) 馃憥
table-create-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table/create/1k duration 107.25 (卤2.55 ms) 107.40 (卤2.20 ms) +0.2ms (0.1%) 馃憣
table-update-10th-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table/update-10th/1k duration 75.00 (卤5.55 ms) 70.10 (卤3.40 ms) -4.9ms (6.5%) 馃憤
tablecmp-append-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table-component/append/1k duration 227.75 (卤10.00 ms) 223.65 (卤14.00 ms) -4.1ms (1.8%) 馃憣
tablecmp-clear-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table-component/clear/1k duration 6.05 (卤1.10 ms) 6.30 (卤1.10 ms) +0.2ms (4.1%) 馃憥
tablecmp-create-10k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table-component/create/10k duration 1594.30 (卤12.35 ms) 1641.50 (卤15.10 ms) +47.2ms (3.0%) 馃憥
tablecmp-create-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table-component/create/1k duration 186.45 (卤3.90 ms) 190.50 (卤5.65 ms) +4.1ms (2.2%) 馃憥
tablecmp-update-10th-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table-component/update-10th/1k duration 67.55 (卤4.05 ms) 67.75 (卤4.35 ms) +0.2ms (0.3%) 馃憣
wc-append-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table-wc/append/1k duration 229.60 (卤9.65 ms) 232.35 (卤12.30 ms) +2.8ms (1.2%) 馃憣
wc-clear-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table-wc/clear/1k duration 10.65 (卤1.75 ms) 11.10 (卤1.90 ms) +0.5ms (4.2%) 馃憣
wc-create-10k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table-wc/create/10k duration 1864.35 (卤13.20 ms) 1914.30 (卤16.85 ms) +50.0ms (2.7%) 馃憥
wc-create-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table-wc/create/1k duration 220.65 (卤4.30 ms) 222.30 (卤4.75 ms) +1.7ms (0.7%) 馃憣
wc-update-10th-1k metric base(d5e4be3) target(a55fdc4) trend
benchmark-table-wc/update-10th/1k duration 67.60 (卤4.95 ms) 67.90 (卤4.00 ms) +0.3ms (0.4%) 馃憣
@jodarove jodarove force-pushed the jodarove/track-rfc-implementation branch 2 times, most recently from 557c49e to 48413b4 Aug 7, 2019
@salesforce-best-lwc-internal

This comment has been minimized.

Copy link

salesforce-best-lwc-internal bot commented Aug 7, 2019

Benchmark results

Base commit: a40d174 | Target commit: 48413b4

lwc-engine-benchmark

table-append-1k metric base(a40d174) target(48413b4) trend
benchmark-table/append/1k duration 140.90 (卤3.90 ms) 142.55 (卤4.35 ms) +1.7ms (1.2%) 馃憣
table-clear-1k metric base(a40d174) target(48413b4) trend
benchmark-table/clear/1k duration 10.35 (卤1.05 ms) 10.60 (卤0.75 ms) +0.2ms (2.4%) 馃憣
table-create-10k metric base(a40d174) target(48413b4) trend
benchmark-table/create/10k duration 849.00 (卤6.10 ms) 846.25 (卤4.35 ms) -2.8ms (0.3%) 馃憣
table-create-1k metric base(a40d174) target(48413b4) trend
benchmark-table/create/1k duration 108.80 (卤2.75 ms) 107.40 (卤2.35 ms) -1.4ms (1.3%) 馃憤
table-update-10th-1k metric base(a40d174) target(48413b4) trend
benchmark-table/update-10th/1k duration 75.65 (卤4.40 ms) 76.10 (卤4.75 ms) +0.4ms (0.6%) 馃憣
tablecmp-append-1k metric base(a40d174) target(48413b4) trend
benchmark-table-component/append/1k duration 225.15 (卤10.45 ms) 225.80 (卤14.40 ms) +0.7ms (0.3%) 馃憣
tablecmp-clear-1k metric base(a40d174) target(48413b4) trend
benchmark-table-component/clear/1k duration 6.20 (卤1.00 ms) 6.30 (卤1.05 ms) +0.1ms (1.6%) 馃憣
tablecmp-create-10k metric base(a40d174) target(48413b4) trend
benchmark-table-component/create/10k duration 1627.95 (卤17.20 ms) 1603.35 (卤8.95 ms) -24.6ms (1.5%) 馃憤
tablecmp-create-1k metric base(a40d174) target(48413b4) trend
benchmark-table-component/create/1k duration 186.35 (卤4.75 ms) 185.40 (卤4.85 ms) -0.9ms (0.5%) 馃憣
tablecmp-update-10th-1k metric base(a40d174) target(48413b4) trend
benchmark-table-component/update-10th/1k duration 68.10 (卤4.55 ms) 70.15 (卤4.60 ms) +2.1ms (3.0%) 馃憣
wc-append-1k metric base(a40d174) target(48413b4) trend
benchmark-table-wc/append/1k duration 232.85 (卤12.95 ms) 229.15 (卤10.30 ms) -3.7ms (1.6%) 馃憣
wc-clear-1k metric base(a40d174) target(48413b4) trend
benchmark-table-wc/clear/1k duration 10.65 (卤1.85 ms) 10.50 (卤1.85 ms) -0.1ms (1.4%) 馃憣
wc-create-10k metric base(a40d174) target(48413b4) trend
benchmark-table-wc/create/10k duration 1840.60 (卤14.30 ms) 1863.40 (卤15.90 ms) +22.8ms (1.2%) 馃憥
wc-create-1k metric base(a40d174) target(48413b4) trend
benchmark-table-wc/create/1k duration 227.85 (卤3.55 ms) 223.05 (卤3.95 ms) -4.8ms (2.1%) 馃憤
wc-update-10th-1k metric base(a40d174) target(48413b4) trend
benchmark-table-wc/update-10th/1k duration 66.85 (卤5.55 ms) 66.95 (卤5.90 ms) +0.1ms (0.1%) 馃憣
@jodarove jodarove requested review from diervo, pmdartus and caridy Aug 7, 2019
@salesforce-best-lwc-internal

This comment has been minimized.

Copy link

salesforce-best-lwc-internal bot commented Aug 7, 2019

Benchmark results

Base commit: a40d174 | Target commit: c95de19

lwc-engine-benchmark

table-append-1k metric base(a40d174) target(c95de19) trend
benchmark-table/append/1k duration 140.90 (卤3.90 ms) 141.45 (卤3.00 ms) +0.5ms (0.4%) 馃憣
table-clear-1k metric base(a40d174) target(c95de19) trend
benchmark-table/clear/1k duration 10.35 (卤1.05 ms) 10.70 (卤0.90 ms) +0.3ms (3.4%) 馃憣
table-create-10k metric base(a40d174) target(c95de19) trend
benchmark-table/create/10k duration 849.00 (卤6.10 ms) 850.55 (卤5.45 ms) +1.5ms (0.2%) 馃憣
table-create-1k metric base(a40d174) target(c95de19) trend
benchmark-table/create/1k duration 108.80 (卤2.75 ms) 108.90 (卤3.45 ms) +0.1ms (0.1%) 馃憣
table-update-10th-1k metric base(a40d174) target(c95de19) trend
benchmark-table/update-10th/1k duration 75.65 (卤4.40 ms) 74.75 (卤5.35 ms) -0.9ms (1.2%) 馃憣
tablecmp-append-1k metric base(a40d174) target(c95de19) trend
benchmark-table-component/append/1k duration 225.15 (卤10.45 ms) 229.80 (卤9.10 ms) +4.7ms (2.1%) 馃憣
tablecmp-clear-1k metric base(a40d174) target(c95de19) trend
benchmark-table-component/clear/1k duration 6.20 (卤1.00 ms) 6.35 (卤0.90 ms) +0.1ms (2.4%) 馃憣
tablecmp-create-10k metric base(a40d174) target(c95de19) trend
benchmark-table-component/create/10k duration 1627.95 (卤17.20 ms) 1632.05 (卤15.55 ms) +4.1ms (0.3%) 馃憣
tablecmp-create-1k metric base(a40d174) target(c95de19) trend
benchmark-table-component/create/1k duration 186.35 (卤4.75 ms) 188.10 (卤4.45 ms) +1.8ms (0.9%) 馃憣
tablecmp-update-10th-1k metric base(a40d174) target(c95de19) trend
benchmark-table-component/update-10th/1k duration 68.10 (卤4.55 ms) 69.50 (卤6.10 ms) +1.4ms (2.1%) 馃憣
wc-append-1k metric base(a40d174) target(c95de19) trend
benchmark-table-wc/append/1k duration 232.85 (卤12.95 ms) 227.95 (卤9.25 ms) -4.9ms (2.1%) 馃憣
wc-clear-1k metric base(a40d174) target(c95de19) trend
benchmark-table-wc/clear/1k duration 10.65 (卤1.85 ms) 11.05 (卤1.90 ms) +0.4ms (3.8%) 馃憣
wc-create-10k metric base(a40d174) target(c95de19) trend
benchmark-table-wc/create/10k duration 1840.60 (卤14.30 ms) 1855.40 (卤18.25 ms) +14.8ms (0.8%) 馃憥
wc-create-1k metric base(a40d174) target(c95de19) trend
benchmark-table-wc/create/1k duration 227.85 (卤3.55 ms) 225.65 (卤5.60 ms) -2.2ms (1.0%) 馃憣
wc-update-10th-1k metric base(a40d174) target(c95de19) trend
benchmark-table-wc/update-10th/1k duration 66.85 (卤5.55 ms) 66.10 (卤4.95 ms) -0.8ms (1.1%) 馃憣
@@ -89,6 +89,7 @@ export interface UninitializedVM {
cmpProps: any;
cmpSlots: SlotSet;
cmpTrack: any;
cmpFields: any;

This comment has been minimized.

Copy link
@caridy

caridy Aug 9, 2019

Contributor

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.

@@ -231,6 +232,7 @@ export function createVM(elm: HTMLElement, Ctor: ComponentConstructor, options:
context: create(null),
cmpProps: create(null),
cmpTrack: create(null),
cmpFields: create(null),

This comment has been minimized.

Copy link
@caridy

caridy Aug 9, 2019

Contributor

remove

@@ -154,6 +158,7 @@ function createComponentDef(
name,
wire,
track,
fields,

This comment has been minimized.

Copy link
@caridy

caridy Aug 9, 2019

Contributor

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.

import { defineProperty, isFalse } from '../shared/language';

export function observeFields(Ctor: ComponentConstructor, fields: string[] | undefined) {
if (fields) {

This comment has been minimized.

Copy link
@caridy

caridy Aug 9, 2019

Contributor

!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.

This comment has been minimized.

Copy link
@caridy

caridy Aug 9, 2019

Contributor

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.

This comment has been minimized.

Copy link
@caridy

caridy Aug 9, 2019

Contributor

you can rename this method to createObservableFieldsMap(fields: PropertyKey[]): PropertyDescriptorMap

!isRendering,
`${vmBeingRendered}.render() method has side effects on the state of ${vm}.${String(
key
)}`

This comment has been minimized.

Copy link
@caridy

caridy Aug 9, 2019

Contributor

... state of foo field.

Copy link
Contributor

caridy left a comment

few minor stylish changes... but the logic is ready.

jodarove added 3 commits Aug 7, 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.
@salesforce-best-lwc-internal

This comment has been minimized.

Copy link

salesforce-best-lwc-internal bot commented Aug 9, 2019

Benchmark results

Base commit: a0a0862 | Target commit: 4a6a167

@jodarove jodarove force-pushed the jodarove/track-rfc-implementation branch from 4a6a167 to f4bb16d Aug 9, 2019
import { isFalse } from '../shared/language';

export function createObservableFieldsDescriptorMap(fields: PropertyKey[]): PropertyDescriptorMap {
return fields.reduce((acc, field) => {

This comment has been minimized.

Copy link
@jodarove

jodarove Aug 9, 2019

Author Contributor

Use ArrayReduce from language

Copy link
Member

ekashida left a 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/engine/src/framework/def.ts 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(

This comment has been minimized.

Copy link
@ekashida

ekashida Aug 9, 2019

Member

Nit: Isn't the call to String(key) implicit?

This comment has been minimized.

Copy link
@jodarove

jodarove Aug 9, 2019

Author Contributor

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 <ekashida@gmail.com>

Update packages/integration-karma/test/component/observed-fields/index.spec.js

Co-Authored-By: Eugene Kashida <ekashida@gmail.com>

Update packages/@lwc/engine/src/framework/def.ts

Co-Authored-By: Eugene Kashida <ekashida@gmail.com>

Update packages/@lwc/engine/src/framework/observable-fields.ts

Co-Authored-By: Eugene Kashida <ekashida@gmail.com>

Update packages/@lwc/engine/src/framework/observable-fields.ts

Co-Authored-By: Eugene Kashida <ekashida@gmail.com>

Update packages/@lwc/babel-plugin-component/src/post-process/transform.js

Co-Authored-By: Eugene Kashida <ekashida@gmail.com>
@jodarove jodarove force-pushed the jodarove/track-rfc-implementation branch from e50a78b to 726f03c Aug 9, 2019
@jodarove

This comment has been minimized.

Copy link
Contributor Author

jodarove commented Aug 9, 2019

@ekashida @caridy this ready for another pass.

@ekashida yes, i agree with you, registerDecorators is looking more to registerComponentMeta, i believe @caridy idea is to tackle this in another effort.

}

export interface DecoratorMeta {
wire: WireHash | undefined;
track: TrackDef;
props: PropsDef;
methods: MethodDef;
fields?: string[];

This comment has been minimized.

Copy link
@caridy

caridy Aug 10, 2019

Contributor

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
caridy approved these changes Aug 10, 2019
Copy link
Contributor

caridy left a comment

small nip, let's roll! this is good to go!

Copy link
Member

ekashida left a comment

nice work!

@ekashida

This comment has been minimized.

Copy link
Member

ekashida commented Aug 10, 2019

Where are the perf numbers?

@caridy

This comment has been minimized.

Copy link
Contributor

caridy commented Aug 11, 2019

@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.

@@ -162,6 +162,7 @@ function createComponentDef(
name,
wire,
track,
fields: undefined,

This comment has been minimized.

Copy link
@caridy

caridy Aug 12, 2019

Contributor

this is not needed right? we can remove this from Def completely.

This comment has been minimized.

Copy link
@jodarove

jodarove Aug 12, 2019

Author Contributor

@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.
@jodarove jodarove merged commit 2dcaa8c into master Aug 12, 2019
11 checks passed
11 checks passed
best Best Summary
Details
build_and_test Workflow: build_and_test
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: perf_and_compare Your tests passed on CircleCI!
Details
ci/circleci: push_canary_npm_artifacts Your tests passed on CircleCI!
Details
ci/circleci: test_integration Your tests passed on CircleCI!
Details
ci/circleci: test_integration_compat Your tests passed on CircleCI!
Details
ci/circleci: test_karma Your tests passed on CircleCI!
Details
ci/circleci: test_unit Your tests passed on CircleCI!
Details
ci/conventional-title Title format complies with conventional-commits!
Details
salesforce-cla All contributors have signed the CLA
Details
@jodarove jodarove deleted the jodarove/track-rfc-implementation branch Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.