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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Apr 10, 2020
Merged

Conversation

caridy
Copy link
Contributor

@caridy caridy commented Aug 21, 2019

Details

RFC for this change: salesforce/lwc-rfcs#14

NOTE: RFC for the wire reform needs to be approved before we can merge this PR.

Does this PR introduce breaking changes?

  • 🚨 Yes, it does introduce breaking changes.
  1. Current wire adapterIds defined as Symbol will be broken with this update.
  • Upgrade path:
export const adapterId = Symbol('AdapterId');

// should change to:

export const adapterId = () => {
    throw new Error("Imperative use is not supported. Use @wire(adapterId)");
};

GUS work item

W-6567761

@caridy caridy changed the title Caridy/wire reform 2 refactor(wire-service): wire decorator reform Aug 21, 2019
@caridy caridy force-pushed the caridy/wire-reform-2 branch from aefc8ef to 6bcf0be Compare August 28, 2019 16:23
invokeServiceHook(vm, wiring);
}
// initializing the wire decorator per instance only when really needed
if (vm.def.wire.length > 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this a def's hook function

@@ -586,6 +586,112 @@ describe('Transform property', () => {
}
);

pluginTest(
"config function should use bracket notation for param when it's definition has invalid identifier as segment",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's open an issue to se if we can eventually make this to throw.

);

pluginTest(
'config function should use bracket notation when param definition has empty segment',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here;... it should eventually be a invalid grammar error.

@@ -264,7 +265,10 @@ export function createVM(elm: HTMLElement, Ctor: ComponentConstructor, options:

// link component to the wire service
const initializedVm = uninitializedVm as VM;
linkComponent(initializedVm);
// initializing the wire decorator per instance only when really needed
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this to the lightning element constructor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we install the wire adapters, we need to call the config function that will pull the data from the component. If we move this to the element constructor, those values are not yet initialized, therefore we will get the assertion message.

@jodarove jodarove force-pushed the caridy/wire-reform-2 branch from 5c0d451 to 00b56c9 Compare October 8, 2019 21:45
@jodarove jodarove force-pushed the caridy/wire-reform-2 branch from 00b56c9 to 315098c Compare October 15, 2019 22:05
@@ -76,16 +76,6 @@ export function createComponent(uninitializedVm: UninitializedVM, Ctor: Componen

export function getTemplateReactiveObserver(vm: VM): ReactiveObserver {
return new ReactiveObserver(() => {
if (process.env.NODE_ENV !== 'production') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happen with this check @jodarove ?

@caridy
Copy link
Contributor Author

caridy commented Feb 1, 2020

@trevor-bliss I don't think that's broken with the wire-reform since all existing adapters are still using the legacy api, just like that package. Can you elaborate more on how is this going to be broken initially?

@trevor-bliss
Copy link
Contributor

@caridy Ah missed that the legacy API is still in tact here. I see the getTodo integration test adapter that looks like it's doing what the test util does. Should be fine.

@jodarove jodarove force-pushed the caridy/wire-reform-2 branch from 01b611a to 25bc418 Compare February 3, 2020 19:02
@jodarove
Copy link
Contributor

jodarove commented Feb 4, 2020

however, @caridy there is an issue with jest and the wire-service-util: the register call happens after the adapter has being saved by the engine in registerDecorators.

From the documentation, when testing a component using @wire:

import { createElement } from 'lwc';
import { registerLdsTestWireAdapter } from '@salesforce/sfdx-lwc-jest';

// importing the component executes registerDecorators and saves the incorrect adapter,
// because at this point in jest the register have not being called yet.
import ProductCard from 'c/productCard';

import { getRecord } from 'lightning/uiRecordApi';


// The register from wire-service is called here, and adds the `adapter` to the `adapterId` .
const getRecordWireAdapter = registerLdsTestWireAdapter(getRecord);

// later on, this will fail, because the engine cant create an adapter instance (or the created instance will not have the expected methods)
const element = createElement('c-product_filter', { is: ProductCard });
document.body.appendChild(element);

cc/ @trevor-bliss since you may have more insight

@caridy
Copy link
Contributor Author

caridy commented Feb 5, 2020

LGTM @jodarove

@jodarove jodarove force-pushed the caridy/wire-reform-2 branch from a63113f to c90e12d Compare February 11, 2020 05:38
@@ -63,6 +167,14 @@ function buildWireConfigValue(t, wiredValues) {
wireConfig.push(t.objectProperty(t.identifier('method'), t.numericLiteral(1)));
}

wireConfig.push(
t.objectProperty(
t.identifier('hasParams'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need an extra parameter to know if the wire config has dynamic params?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmdartus with this reform there is no need of params or static in the wire meta. it hasn't being removed because of the probable impact in the platform metadata compiler.

once this pr is merged we will proceed to update the platform metadata compiler and remove the params and static meta from the wire definition.

@salesforce-best-lwc-internal
Copy link

🥳 Performance Improvement

Best has detected that there is a 8.4% performance improvement across your benchmarks.

Please click here to see more details.

Click to view significantly changed benchmarks

lwc-engine-benchmark

✅ Improvements base (85446eb) target (89923cd) trend
table-append-1k.benchmark/benchmark-table/append/1k 268.88 (± 2.51ms) 195.03 (± 2.60ms) -73.9ms (27.5%)
table-create-10k.benchmark/benchmark-table/create/10k 1501.89 (± 17.61ms) 1121.26 (± 7.52ms) -380.6ms (25.3%)
table-create-1k.benchmark/benchmark-table/create/1k 159.00 (± 1.39ms) 135.59 (± 4.00ms) -23.4ms (14.7%)
table-update-10th-1k.benchmark/benchmark-table/update-10th/1k 133.74 (± 1.91ms) 100.97 (± 1.55ms) -32.8ms (24.5%)
tablecmp-append-1k.benchmark/benchmark-table-component/append/1k 397.87 (± 2.57ms) 296.67 (± 6.90ms) -101.2ms (25.4%)
tablecmp-create-1k.benchmark/benchmark-table-component/create/1k 257.53 (± 2.66ms) 238.73 (± 4.89ms) -18.8ms (7.3%)
tablecmp-update-10th-1k.benchmark/benchmark-table-component/update-10th/1k 133.86 (± 0.86ms) 96.40 (± 2.51ms) -37.5ms (28.0%)
wc-append-1k.benchmark/benchmark-table-wc/append/1k 609.76 (± 8.99ms) 442.00 (± 4.75ms) -167.8ms (27.5%)
wc-create-10k.benchmark/benchmark-table-wc/create/10k 4628.40 (± 168.35ms) 3793.13 (± 22.91ms) -835.3ms (18.0%)
wc-create-1k.benchmark/benchmark-table-wc/create/1k 458.99 (± 4.51ms) 392.32 (± 4.65ms) -66.7ms (14.5%)
wc-update-10th-1k.benchmark/benchmark-table-wc/update-10th/1k 138.53 (± 1.28ms) 95.13 (± 1.76ms) -43.4ms (31.3%)
❌ Regressions base (85446eb) target (89923cd) trend
table-clear-1k.benchmark/benchmark-table/clear/1k 11.77 (± 0.18ms) 14.22 (± 0.41ms) +2.5ms (20.8%)
tablecmp-clear-1k.benchmark/benchmark-table-component/clear/1k 5.50 (± 0.13ms) 7.13 (± 0.18ms) +1.6ms (29.4%)
wc-clear-1k.benchmark/benchmark-table-wc/clear/1k 30.49 (± 0.42ms) 42.27 (± 1.09ms) +11.8ms (38.7%)

@jodarove jodarove force-pushed the caridy/wire-reform-2 branch from 89923cd to 0158f39 Compare March 23, 2020 21:38
@salesforce-best-lwc-internal
Copy link

🥳 Performance Improvement

Best has detected that there is a 9.8% performance improvement across your benchmarks.

Please click here to see more details.

Click to view significantly changed benchmarks

lwc-engine-benchmark

✅ Improvements base (85446eb) target (0158f39) trend
table-append-1k.benchmark/benchmark-table/append/1k 268.88 (± 2.51ms) 195.67 (± 2.00ms) -73.2ms (27.2%)
table-create-10k.benchmark/benchmark-table/create/10k 1501.89 (± 17.61ms) 1090.90 (± 9.69ms) -411.0ms (27.4%)
table-create-1k.benchmark/benchmark-table/create/1k 159.00 (± 1.39ms) 138.26 (± 6.16ms) -20.7ms (13.0%)
table-update-10th-1k.benchmark/benchmark-table/update-10th/1k 133.74 (± 1.91ms) 103.71 (± 2.08ms) -30.0ms (22.5%)
tablecmp-append-1k.benchmark/benchmark-table-component/append/1k 397.87 (± 2.57ms) 293.21 (± 2.34ms) -104.7ms (26.3%)
tablecmp-create-1k.benchmark/benchmark-table-component/create/1k 257.53 (± 2.66ms) 238.07 (± 4.59ms) -19.5ms (7.6%)
tablecmp-update-10th-1k.benchmark/benchmark-table-component/update-10th/1k 133.86 (± 0.86ms) 89.42 (± 1.63ms) -44.4ms (33.2%)
wc-append-1k.benchmark/benchmark-table-wc/append/1k 609.76 (± 8.99ms) 429.62 (± 4.99ms) -180.1ms (29.5%)
wc-create-10k.benchmark/benchmark-table-wc/create/10k 4628.40 (± 168.35ms) 3837.20 (± 36.18ms) -791.2ms (17.1%)
wc-create-1k.benchmark/benchmark-table-wc/create/1k 458.99 (± 4.51ms) 393.90 (± 3.02ms) -65.1ms (14.2%)
wc-update-10th-1k.benchmark/benchmark-table-wc/update-10th/1k 138.53 (± 1.28ms) 91.51 (± 2.19ms) -47.0ms (33.9%)
❌ Regressions base (85446eb) target (0158f39) trend
table-clear-1k.benchmark/benchmark-table/clear/1k 11.77 (± 0.18ms) 14.04 (± 0.49ms) +2.3ms (19.3%)
tablecmp-clear-1k.benchmark/benchmark-table-component/clear/1k 5.50 (± 0.13ms) 6.00 (± 0.12ms) +0.5ms (9.0%)
wc-clear-1k.benchmark/benchmark-table-wc/clear/1k 30.49 (± 0.42ms) 42.97 (± 0.71ms) +12.5ms (40.9%)

@jodarove jodarove force-pushed the caridy/wire-reform-2 branch 2 times, most recently from ce18b81 to de0f081 Compare March 23, 2020 23:38
@jodarove jodarove force-pushed the caridy/wire-reform-2 branch 2 times, most recently from 4f4d5c8 to a089973 Compare April 10, 2020 03:47
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
@jodarove jodarove merged commit 9e19ce5 into master Apr 10, 2020
@jodarove jodarove deleted the caridy/wire-reform-2 branch April 10, 2020 23:22
jodarove added a commit that referenced this pull request Apr 14, 2020
fixes an issue introduced on #1459 that calls the update method on a wire adapter, even when initially all the params of the config are undefined. The fix is only for the legacy adapters that are registered with the wire-service.
jodarove added a commit that referenced this pull request Apr 17, 2020
fixes an issue introduced on #1459 that calls the update method on a wire adapter, even when initially all the params of the config are undefined. The fix is only for the legacy adapters that are registered with the wire-service.
jodarove added a commit that referenced this pull request Apr 17, 2020
* Revert "fix: Wire service call config when all params are undefined"

This reverts commit f74a132.

* fix(wire-service): config with all params being undefined (#1823)

fixes an issue introduced on #1459 that calls the update method on a wire adapter, even when initially all the params of the config are undefined. The fix is only for the legacy adapters that are registered with the wire-service.

* test: Fix failures due to Chrome 81 and Firefox 75 releases (#1819)

* test: disable ariaSelected reflection test

* test: update test for dispatchEvent type error (#1820)

* test: relax from TypeError to Error

* chore: review feedback

Co-authored-by: Eugene Kashida <ekashida@gmail.com>

Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com>
Co-authored-by: Eugene Kashida <ekashida@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants