feat(wire-service): imported function identifier as adapter id for @wire #26

yungcheng · 2018-01-22T23:33:00Z

PR Checklist

What kind of change does this PR introduce? (add 'x' - [x])

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:
Please check if your PR fulfills the following requirements:

The PR fulfills these requirements:

Tests for the changes have been added (for bug fixes / features)
Both unit and integration tests pass
Docs have been added / updated (for bug fixes / features)

The PR title follows conventional commit format:
commit-type(optional scope): commit description.

Supported commit types: build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test.
Supported scope: The scope should be the name of the npm package affected (engine, compiler, wire-service, etc.)

More details on LWC semantic commit can be found here.

Other information:

salesforce-best-lwc-internal · 2018-01-22T23:39:15Z

Benchmark comparison

Base commit: 0a8e11f | Target commit: ade0071

benchmark	base(`0a8e11f`)	target(`ade0071`)	trend
append-1k.benchmark:benchmark-table/add/1k	335.93 (± 9.16 ms)	330.90 (± 9.16 ms)	👌
clear-1k.benchmark:benchmark-table/clear/1k	51.34 (± 1.54 ms)	50.42 (± 1.54 ms)	👌
create-1k.benchmark:benchmark-table/create/1k	199.85 (± 3.13 ms)	191.38 (± 3.13 ms)	👍
update-10th-1k.benchmark:benchmark-table/update-10th/1k	182.35 (± 2.63 ms)	193.34 (± 2.63 ms)	👎

pmdartus · 2018-01-22T23:56:30Z

packages/babel-plugin-transform-lwc-class/src/decorators/api/validate.js

@@ -76,7 +76,7 @@ function validatePairSetterGetter(decorators) {
    });
 }

-module.exports = function validate(klass, decorators) {
+module.exports = function validate(klass, decorators, importSpecifiers) {


ImportSpecifiers is not used here.

pmdartus · 2018-01-22T23:57:48Z

packages/babel-plugin-transform-lwc-class/src/decorators/track/index.js

@@ -7,7 +7,7 @@ function isTrackDecorator(decorator) {
    return decorator.name === TRACK_DECORATOR;
 }

-function validate(klass, decorators) {
+function validate(klass, decorators, importSpecifiers) {


importSpecifiers is not used here.

pmdartus · 2018-01-22T23:59:44Z

packages/babel-plugin-transform-lwc-class/src/decorators/wire/transform.js

+
+        // TODO: deprecate type (string as adapter id once consumer has migrated to use imported identifier)
+        if (id.isStringLiteral()) {
+            Object.defineProperty(ret, 'type', {


Using Object.defineProperty is not needed here a simple property set would do the trick.

pmdartus · 2018-01-23T00:20:49Z

packages/babel-plugin-transform-lwc-class/src/decorators/wire/validate.js

-        throw id.buildCodeFrameError(
-            `@wire expects a string as first parameter.`
-        );
+        if (!id.isIdentifier()) {


You should simplify the logic here:

if (!id.isStringLiteral() || !id.isIdentifier()) { // throw because unexpected adapter } if (id.isIdentifier() && !importSpecifiers.includes(id.node.name)) { // throw because adapter should be imported }

pmdartus · 2018-01-23T00:24:04Z

packages/babel-plugin-transform-lwc-class/src/utils.js

@@ -56,7 +56,7 @@ function getImportsStatements(path, sourceName) {

    return programPath.get('body').filter(node => (
        node.isImportDeclaration() &&
-        node.get('source').isStringLiteral({ value: sourceName })
+        (!sourceName || node.get('source').isStringLiteral({ value: sourceName }))


The association of AND and OR condition is not obvious, can you break it down into multiple lines?

pmdartus · 2018-01-23T00:27:58Z

packages/babel-plugin-transform-lwc-class/src/utils.js

@@ -68,7 +68,7 @@ function getImportSpecifiers(path, sourceName) {
        return [...acc, ...importStatement.get('specifiers')];
    }, []).reduce((acc, specifier) => {
        // Get the list of specifiers with their name
-        const imported = specifier.get('imported').node.name;
+        const imported = specifier.get('imported').node && specifier.get('imported').node.name;


If specifier.get('imported').node is undefined it will add to the array { name: undefined, path: specifier }. The name property should always be a string.

Do you have an example where specifier.get('imported').node returns undefined?

import * as target from '...';, it is discovered from a test

pmdartus · 2018-01-23T00:30:39Z

packages/babel-plugin-transform-lwc-class/src/decorators/index.js

@@ -120,6 +120,7 @@ function removeImportSpecifiers(specifiers) {
 module.exports = function decoratorVisitor({ types: t }) {
    return {
        Program(path, state) {
+            const importSpecifiers = getImportSpecifiers(path).map(i => i.name);
            const engineImportSpecifiers = getImportSpecifiers(path, LWC_PACKAGE_ALIAS);


Instead of doing 2 passes to retrieve imports from engine and all the imports. I would be faster to retrieve all the imports and then filter based on the module source.

pmdartus · 2018-01-23T00:34:06Z

packages/babel-plugin-transform-lwc-class/src/decorators/index.js

@@ -120,6 +120,7 @@ function removeImportSpecifiers(specifiers) {
 module.exports = function decoratorVisitor({ types: t }) {
    return {
        Program(path, state) {
+            const importSpecifiers = getImportSpecifiers(path).map(i => i.name);


In the scope you have:

importSpecifiers: list of imported properties as string

engineImportSpecifiers: list of imported properties as a data structure containing the specifier and the name

The naming of the variables should reflect that they are not referencing the same king of data structures.

pmdartus · 2018-01-23T00:37:25Z

packages/babel-plugin-transform-lwc-class/src/decorators/wire/validate.js

+            );
+        } else {
+            // identifier has to be imported
+            if (!importSpecifiers.includes(id.node.name)) {


This is not enough to ensure that the wires are imported. For example:

import { record } from 'lds'; export function factory() { // Shadow imported identifier const record = 'another-adapter'; return class Test { @api(record, {}) wiredRecord; } }

In order to solve this issue, you need to locate where the identifier is defined. The scope property on the path holds this kind of information.

pmdartus · 2018-01-23T00:39:16Z

packages/lwc-wire-service/src/__tests__/wire-service.test.js

+            const wireDef = { adapter: unknownFunc };
+            expect(() => {
+                target.getAdapter(wireDef, "target");
+            }).toThrow();


You should probably assert the thrown message.

yungcheng · 2018-01-23T02:42:48Z

@pmdartus, question for you:
for import record from 'data-service';, does lwc-compiler need to output data-service in the dependencies metadata? wire-service doesn't really need the hard dependency because it has the ability to lookup adapter, but I assume this is something we want to enforce to enable static analysis?

@kevinv11n thoughts?

kevinv11n · 2018-01-23T05:09:35Z

Re for import record from 'data-service';, does lwc-compiler need to output data-service in the dependencies metadata -- I'm working through the current + roadmap'ed UIAPI/LDS resources to figure out how we want this code organized. We may need to adjust things a bit.

yungcheng · 2018-01-24T04:58:14Z

@pmdartus second pass, now no longer need to keep a list fo imported identifiers

salesforce-best-lwc-internal · 2018-01-24T05:00:07Z

Benchmark comparison

Base commit: b5fc0ed | Target commit: e4e92fe

benchmark	base(`b5fc0ed`)	target(`e4e92fe`)	trend
append-1k.benchmark:benchmark-table/add/1k	328.81 (± 11.44 ms)	337.14 (± 11.44 ms)	👎
clear-1k.benchmark:benchmark-table/clear/1k	48.70 (± 1.38 ms)	49.53 (± 1.38 ms)	👎
create-1k.benchmark:benchmark-table/create/1k	195.45 (± 3.58 ms)	195.54 (± 3.58 ms)	👌
update-10th-1k.benchmark:benchmark-table/update-10th/1k	183.68 (± 3.84 ms)	185.25 (± 3.84 ms)	👌

salesforce-best-lwc-internal · 2018-01-25T18:35:18Z

Benchmark comparison

Base commit: 9dcac43 | Target commit: bca355a

benchmark	base(`9dcac43`)	target(`bca355a`)	trend
append-1k.benchmark:benchmark-table/add/1k	347.06 (± 9.45 ms)	353.71 (± 9.45 ms)	👌
clear-1k.benchmark:benchmark-table/clear/1k	49.52 (± 2.04 ms)	54.46 (± 2.04 ms)	👎
create-1k.benchmark:benchmark-table/create/1k	201.60 (± 7.61 ms)	227.78 (± 7.61 ms)	👎
update-10th-1k.benchmark:benchmark-table/update-10th/1k	186.22 (± 6.43 ms)	188.85 (± 6.43 ms)	👌

salesforce-best-lwc-internal · 2018-01-25T19:40:30Z

Benchmark comparison

Base commit: 9dcac43 | Target commit: 120c13c

benchmark	base(`9dcac43`)	target(`120c13c`)	trend
append-1k.benchmark:benchmark-table/add/1k	347.06 (± 10.22 ms)	336.04 (± 10.22 ms)	👍
clear-1k.benchmark:benchmark-table/clear/1k	49.52 (± 1.27 ms)	51.91 (± 1.27 ms)	👎
create-1k.benchmark:benchmark-table/create/1k	201.60 (± 4.21 ms)	199.54 (± 4.21 ms)	👌
update-10th-1k.benchmark:benchmark-table/update-10th/1k	186.22 (± 4.39 ms)	190.07 (± 4.39 ms)	👌

apapko

Few minor comments. It would probably be a good idea to open up a user story/issue to keep track of existing todos:
// TODO: deprecate type once consumers have migrate to use function identifier for adapter id.

apapko · 2018-01-25T19:56:13Z

packages/babel-plugin-transform-lwc-class/src/decorators/wire.js

                    );
                }

+                let adapterModule;
+                if (id.isIdentifier()) {
+                    const adapterImportPath = path.scope.getBinding(id.node.name).path;


should we check if getBinding doesn't produce undefined prior accessing path?

it should always be defined because id exists, getBinding returns where the id is declared.

apapko · 2018-01-25T19:56:38Z

packages/babel-plugin-transform-lwc-class/src/decorators/wire.js

+                let adapterModule;
+                if (id.isIdentifier()) {
+                    const adapterImportPath = path.scope.getBinding(id.node.name).path;
+                    if (adapterImportPath.isImportSpecifier()) {


existence check on adapterImportPath

again, this should always exist

apapko · 2018-01-25T19:59:05Z

packages/babel-plugin-transform-lwc-class/src/decorators/wire.js

+
+                // TODO: deprecate type (string as adapter id once consumer has migrated to use imported identifier)
+                if (id.isStringLiteral()) {
+                    wiredValue.type = id.node.value;


perhaps we can extract id.node reference into a local variable, there are multiple accesses in this file.

we could, but path.node is the common pattern to access a node in babel plugin

apapko · 2018-01-25T20:02:04Z

packages/lwc-integration/src/shared/todo.js

+        }
+    };
+
+    return {


there will be an error if someone calls next / error/ or complete before calling observable(); Your observer is undefined until subscribe is invoked.

this is not exposed, the only thing that the module exposes is serviceTodo

salesforce-best-lwc-internal · 2018-01-29T17:50:00Z

Benchmark comparison

Base commit: 839cdfe | Target commit: b46ef01

benchmark	base(`839cdfe`)	target(`b46ef01`)	trend

caridy

I'm fine with this baby steps. Eventually, to be able to do real rollups, we will need to add to the metadata the module specifier of the adapter used.

kevinv11n

Looks good, a handle of nits and a question.

kevinv11n · 2018-01-30T22:30:40Z

packages/babel-plugin-transform-lwc-class/src/__tests__/wire-decorator.test.js

-    test('decorator expects a string as first parameter', `
+    test('decorator expects a function identifier as first parameter', `
+        import { wire } from 'engine';
+        import { record } from 'data-service';


nit: s/record/getRecord/. That's the format we're using per https://salesforce.quip.com/tpa5AVyZ21kc

kevinv11n · 2018-01-30T22:30:49Z

packages/babel-plugin-transform-lwc-class/src/__tests__/wire-decorator.test.js

+        }
+    `, `
+        import { wire } from 'engine';
+        import { record } from 'data-service';


kevinv11n · 2018-01-30T22:31:24Z

packages/babel-plugin-transform-lwc-class/src/__tests__/wire-decorator.test.js

+            loc: {
+                line: 4,
+                column: 6,
+            },
    });

    test('decorator expects an oject as second parameter', `


Not you but we have a typo in object (missing b)

kevinv11n · 2018-01-30T22:32:28Z

packages/lwc-integration/src/components/wired/test-wired-prop-suite/wired-prop/wired-prop.js

@@ -1,9 +1,9 @@
 import { Element } from 'engine';
-
+import { serviceTodo } from 'todo';


nit: s/serviceTodo/getTodo/

kevinv11n · 2018-01-30T22:34:51Z

packages/lwc-wire-service/src/__tests__/wire-service.test.js

+        });
+        it("returns with a known function identifier adapter id", () => {
+            const wireDef = { adapter: knownFunc };
+            target.getAdapter(wireDef, "target");


kevinv11n · 2018-01-30T22:46:17Z

packages/lwc-wire-service/src/wire-service.js

+            throw new Error(`Unknown wire adapter id '${wireDef.type}' in ${wireTarget}'s @wire('${wireDef.type}', ...)`);
+        }
+    } else if (wireDef.adapter) {
+        adapter = ADAPTERS.get(wireDef.adapter.name);


Will this handle identically named functions from multiple modules? I don't believe Function.name handles that.

Once we move from a string key to a function we'll need to adjust wire-adapters.js comments. The code looks like it should be fine because it's exclusively using a Map.

yea this is baby step and if you import same function identifier from different modules then it won't work

Ok, want to file that as an issue or tackle it now? FWIW I think it's fine for the time being because we can ensure uniqueness in LDS.

I've some big questions around how this will work with query rollup. For me, the topic is sufficiently complex that I need to spike with functioning code to be confident in the design.

yungcheng requested review from kevinv11n and pmdartus January 22, 2018 23:33

pmdartus requested changes Jan 23, 2018

View reviewed changes

yungcheng force-pushed the vince/update-wire-syntax branch from ade0071 to e4e92fe Compare January 24, 2018 04:53

yungcheng changed the title ~~feat(wire-service): enable using imported function identifier as adapter id for @wire~~ feat(wire-service): imported function identifier as adapter id for @wire Jan 24, 2018

apapko reviewed Jan 25, 2018

View reviewed changes

yungcheng added 6 commits January 29, 2018 09:32

enable @wire to use imported function identifier as adapter id

b020689

enable function identifier as wire adapter id in @wire decorator

d533399

update wire-service getAdapter error message and tests

ffe55db

update wired integration tests to use function identifier

efebc1b

update wired test template

daa9873

make todo module compat with ie11

b46ef01

yungcheng force-pushed the vince/update-wire-syntax branch from 120c13c to b46ef01 Compare January 29, 2018 17:43

caridy approved these changes Jan 30, 2018

View reviewed changes

pmdartus approved these changes Jan 30, 2018

View reviewed changes

yungcheng merged commit 2c5c540 into master Jan 30, 2018

yungcheng deleted the vince/update-wire-syntax branch January 30, 2018 20:39

kevinv11n reviewed Jan 30, 2018

View reviewed changes

		@@ -1,9 +1,9 @@
		import { Element } from 'engine';

		import { serviceTodo } from 'todo';

feat(wire-service): imported function identifier as adapter id for @wire #26

feat(wire-service): imported function identifier as adapter id for @wire #26

Conversation

yungcheng commented Jan 22, 2018

PR Checklist

Other information:

salesforce-best-lwc-internal bot commented Jan 22, 2018

Benchmark comparison

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungcheng commented Jan 23, 2018

kevinv11n commented Jan 23, 2018 • edited Loading

yungcheng commented Jan 24, 2018

salesforce-best-lwc-internal bot commented Jan 24, 2018

Benchmark comparison

salesforce-best-lwc-internal bot commented Jan 25, 2018

Benchmark comparison

salesforce-best-lwc-internal bot commented Jan 25, 2018

Benchmark comparison

apapko left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salesforce-best-lwc-internal bot commented Jan 29, 2018

Benchmark comparison

caridy left a comment

Choose a reason for hiding this comment

kevinv11n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungcheng Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinv11n commented Jan 23, 2018 •

edited

Loading

apapko left a comment •

edited

Loading

yungcheng Jan 30, 2018 •

edited

Loading