refactor(engine): Decouple createElement from engine internals #1751

pmdartus · 2020-03-03T16:38:09Z

Details

This PR decouples the createElement method from the engine internals in preparation from the server-side rendering code split.

Does this PR introduce breaking changes?

✅ No, it does not introduce breaking changes.

The PR fulfills these requirements:

Have tests for the proposed changes been added? ✅
Have you followed these instructions to clearly describe the issue being fixed or feature enhanced? ✅

GUS work item

caridy

great simplification!

packages/@lwc/engine/src/framework/vm.ts

packages/@lwc/engine/src/framework/upgrade.ts

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

salesforce-best-lwc-internal · 2020-03-11T10:23:34Z

⚠ Performance Regression

Best has detected that there is a 5.6% performance regression across your benchmarks.

Please click here to see more details.

Click to view significantly changed benchmarks

lwc-engine-benchmark

✅ Improvements	base (`62d28c7`)	target (`b4757e9`)	trend
wc-clear-1k.benchmark/benchmark-table-wc/clear/1k	28.02 (± 0.33ms)	25.59 (± 0.42ms)	-2.4ms (8.6%)

❌ Regressions	base (`62d28c7`)	target (`b4757e9`)	trend
table-append-1k.benchmark/benchmark-table/append/1k	257.94 (± 2.48ms)	273.27 (± 2.66ms)	+15.3ms (5.9%)
tablecmp-clear-1k.benchmark/benchmark-table-component/clear/1k	5.66 (± 0.18ms)	6.06 (± 0.31ms)	+0.4ms (7.1%)
wc-append-1k.benchmark/benchmark-table-wc/append/1k	502.20 (± 6.83ms)	563.54 (± 7.75ms)	+61.3ms (12.2%)
wc-create-10k.benchmark/benchmark-table-wc/create/10k	3579.41 (± 22.26ms)	3909.39 (± 8.60ms)	+330.0ms (9.2%)
wc-create-1k.benchmark/benchmark-table-wc/create/1k	373.91 (± 3.44ms)	410.86 (± 4.28ms)	+36.9ms (9.9%)
wc-update-10th-1k.benchmark/benchmark-table-wc/update-10th/1k	134.92 (± 1.06ms)	145.44 (± 2.16ms)	+10.5ms (7.8%)

pmdartus · 2020-03-11T15:24:07Z

After investigating the web components benchmarks regressed because all the custom elements add marks and measures in the performance timeline (even in production for the root element). IMO, all the root elements should be treated identically and because of this, we should be ok with introducing this 5% regression.

diervo · 2020-03-11T15:45:30Z

I dont follow your comment about perf, why would this not represent prod in any case?

caridy · 2020-03-11T16:58:16Z

packages/@lwc/engine/src/framework/restrictions.ts

@@ -78,7 +78,7 @@ function portalRestrictionErrorMessage(name: string, type: string) {

 function getNodeRestrictionsDescriptors(
    node: Node,
-    options: RestrictionsOptions
+    options: RestrictionsOptions = {}


up until now, we have been trying to avoid any default value for arguments due to the perf implication in IE11, the transpiled code is very complete (including a loop).

The rest parameters are transpiled using a loop when targetting ES5, it's not the case for default parameter: http://www.typescriptlang.org/play/index.html?target=1#code/GYVwdgxgLglg9mABMOcAUBDAXIgzlAJxjAHMAaRAIx3yNMQF5EByAEwFNgMQAbKZgJSIA3gChEExBAS44PdgDoecEpgqUBAblEBfUaJTpmAC3Y9lgzUA

caridy

LGTM, just small nip about default arg value.

@wire

Revert "Revert "feat: add wire config as function (#1455)" (#1601)" This reverts commit 722979e. rebase from master Squashed commit of the following: commit 0f00e08 Author: Jose David Rodriguez <jodarove@gmail.com> Date: Thu Sep 19 23:13:15 2019 -0700 refactor(wire-service): wire decorator reform Squashed commit of the following: commit fb136b7 Author: Caridy Patiño <caridy@gmail.com> Date: Thu Aug 29 23:59:16 2019 -0400 fix(wire-service): does not accept adapter id to be a symbol commit 91c2d97 Author: Caridy Patiño <caridy@gmail.com> Date: Thu Aug 29 15:04:19 2019 -0400 fix(engine): tests and karma tests fixes commit 719c41e Merge: 3cdf650 8e5035e Author: Caridy Patiño <caridy@gmail.com> Date: Sat Aug 31 14:31:07 2019 -0400 Merge branch 'master' into caridy/wire-reform-2 commit 8e5035e Author: Caridy Patiño <caridy@gmail.com> Date: Fri Aug 30 22:25:49 2019 -0400 refactor: hidden fields instead of internal fields (2) (#1485) * chore: package-unique keys for engine and synthetic shadow * refactor: remove internal fields in favor of hidden fields * fix: avoid returning boolean false when field undefined * test: use hidden fields instead of internal fields * refactor(synthetic-shadow): hidden fields instead of internal fields * chore(synthetic-shadow): "unique" string keys * fix(engine): issue 1299 second attempt * fix(engine): removing hasOwnProperty check * chore: revert changes for vm access by getComponentConstructor commit 9a7f822 Author: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com> Date: Thu Aug 29 14:05:57 2019 -0700 fix: cloneNode() default behavior should match spec (#1480) commit 3cdf650 Author: Caridy Patiño <caridy@gmail.com> Date: Thu Aug 29 05:40:52 2019 -0400 fix(engine): context to work as expected commit 2224bd2 Author: Caridy Patiño <caridy@gmail.com> Date: Thu Aug 29 04:38:45 2019 -0400 fix(engine): karma tests for context providers commit 8b6c978 Merge: d02243b 5d5f7af Author: Caridy Patiño <caridy@gmail.com> Date: Thu Aug 29 04:33:58 2019 -0400 Merge branch 'caridy/wire-reform-2' of github.com:salesforce/lwc into caridy/wire-reform-2 commit 5d5f7af Author: Jose David Rodriguez <jodarove@gmail.com> Date: Fri Aug 30 13:30:34 2019 -0700 fix: observable-fields commit a901a64 Author: Jose David Rodriguez <jodarove@gmail.com> Date: Thu Aug 29 22:41:50 2019 -0700 wip: wire register is broken it was making karma fail commit d02243b Author: Caridy Patiño <caridy@gmail.com> Date: Wed Aug 28 07:46:56 2019 -0400 fix(engine): remove unnecessary comment commit c9ad2c5 Author: Caridy Patiño <caridy@gmail.com> Date: Wed Aug 28 02:59:29 2019 -0400 refactor(engine): context provider options commit 6bcf0be Author: Caridy Patiño <caridy@gmail.com> Date: Tue Jul 16 23:03:56 2019 -0400 fix(engine): @wire() protocol reform RFC fix: rebase issues fix: wire-reform tests (#1524) * fix: invalid syntax error on invalid wire param value When a wire parameter has invalid value (eg: foo..bar, foo.bar[3]) it should (ideally) resolve in a compilation error during validation phase. This is not possible due that platform components may have a param definition which is invalid but passes compilation, and throwing at compile time would break such components. In such cases where the param does not have proper notation, the config generated will use the bracket notation to match the current behavior (that most likely end up resolving that param as undefined). * fix: tests for wire reactive parameters * fix: unit test register and WireAdapter * fix: backward compability when adding config listener * fix: adding general tests for wire adapter * wip: address pr comments and lint errors fix: reactivity of wire params (#1526) * fix: reactivity of wire params * wip: address review comments and run performance * wip: trigger perf measures * fix: remove valueMutated pruning condition * fix: add componentValueObserved for consistency * fix: remove tf because of wired properties being track before the wire reform. * fix: ensure component rerender in compat * fix: tf because registerDecorators happens before registerAdapter * Revert "fix: tf because registerDecorators happens before registerAdapter" This reverts commit b2d1a89. * fix: wire integration tests fix: rebase issues fix: remove tests because wire defs is not part of the public api of getComponentDef fix: add test for inherited methods fix: context in targets hierarchy fix: add registerWireService for backward compability it is a noop. fix(engine): remove assertions added by mistake on rebase fix: rebase conflicts fix: lint errors todos errors. fix: add support for adapters that use wirecontextevent fix: prefix wired element key with deprecated and remove redundant options in descriptor fix: relax adapterId validation to isExtensible instead of instanceof Object. In order to be able to register an adapterId, the only precondition needed is that adapterId is extensible. this commit changes the old validation that was doing instanceof Object. fix: tf because of error message changed fix: debounce config call when creating component with wire also, adds an extra data for the wire decorator, indicating if it has dynamic parameters. fix: incorrect api field config returned from getComponentDef fix: address pr comments fix: include 33b91a2 in the wire reform fix: —strictFunctionTypes issues plus rebase leftovers Revert "refactor(engine): Decouple createElement from engine internals (#1751)" This reverts commit 1ea1d60. # Conflicts: # packages/@lwc/engine/src/framework/def.ts # packages/@lwc/engine/src/framework/hooks.ts # packages/@lwc/engine/src/framework/services.ts # packages/@lwc/engine/src/framework/upgrade.ts # packages/@lwc/engine/src/framework/vm.ts # packages/@lwc/engine/src/framework/wc.ts # packages/@lwc/wire-service/src/index.ts # packages/@lwc/wire-service/src/property-trap.ts # packages/@lwc/wire-service/src/wiring.ts

pmdartus added the work-in-progress Work in progress label Mar 3, 2020

caridy approved these changes Mar 3, 2020

View reviewed changes

pmdartus removed the work-in-progress Work in progress label Mar 4, 2020

pmdartus requested review from ekashida and ravijayaramappa March 4, 2020 07:35

pmdartus added the next-release label Mar 6, 2020

ravijayaramappa reviewed Mar 9, 2020

View reviewed changes

packages/@lwc/engine/src/framework/vm.ts Outdated Show resolved Hide resolved

ravijayaramappa reviewed Mar 9, 2020

View reviewed changes

packages/@lwc/engine/src/framework/upgrade.ts Outdated Show resolved Hide resolved

ravijayaramappa reviewed Mar 9, 2020

View reviewed changes

packages/@lwc/engine/src/framework/def.ts Show resolved Hide resolved

pmdartus force-pushed the pmdartus/entry-point-clean-up branch from 959fda0 to 6fa7994 Compare March 11, 2020 08:03

pmdartus added 8 commits March 11, 2020 09:04

wip(engine): move restriction to create VM

9260e2e

wip(engine): move global meausurement to VM

26f115e

wip(engine): resolve circular dependencies in def only

dafadc5

wip(engine): remove VM from createVM

36bfe64

wip(engine): simplify expression for mode detection

6fa7994

wip(engine): move custom element patching in base element

3bd6435

wip(engine): remove closure for upgrade

83de7ae

wip(engine): make weakmap lookup first in getComponentDef

b4757e9

pmdartus requested a review from ravijayaramappa March 11, 2020 15:24

caridy reviewed Mar 11, 2020

View reviewed changes

caridy approved these changes Mar 11, 2020

View reviewed changes

ravijayaramappa approved these changes Mar 11, 2020

View reviewed changes

apapko merged commit 1ea1d60 into master Mar 12, 2020

apapko deleted the pmdartus/entry-point-clean-up branch March 12, 2020 17:27

jodarove mentioned this pull request Apr 6, 2020

refactor(wire-service): wire decorator reform #1802

Closed

pmdartus mentioned this pull request Apr 7, 2020

refactor(engine): node-reactions using native web components #1798

Closed

This was referenced Apr 15, 2020

fix: use resolved constructor during creating of template elements #1828

Merged

fix: cache definition for constructor with circular dependency #1842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(engine): Decouple createElement from engine internals #1751

refactor(engine): Decouple createElement from engine internals #1751

pmdartus commented Mar 3, 2020

caridy left a comment

salesforce-best-lwc-internal bot commented Mar 11, 2020

lwc-engine-benchmark

pmdartus commented Mar 11, 2020

diervo commented Mar 11, 2020

caridy Mar 11, 2020

pmdartus Mar 12, 2020

caridy left a comment

refactor(engine): Decouple createElement from engine internals #1751

refactor(engine): Decouple createElement from engine internals #1751

Conversation

pmdartus commented Mar 3, 2020

Details

Does this PR introduce breaking changes?

The PR fulfills these requirements:

GUS work item

caridy left a comment

Choose a reason for hiding this comment

salesforce-best-lwc-internal bot commented Mar 11, 2020

⚠ Performance Regression

lwc-engine-benchmark

pmdartus commented Mar 11, 2020

diervo commented Mar 11, 2020

caridy Mar 11, 2020

Choose a reason for hiding this comment

pmdartus Mar 12, 2020

Choose a reason for hiding this comment

caridy left a comment

Choose a reason for hiding this comment