feat: augments metadata gathering for @wire decorator #631

ghost · 2018-09-11T17:15:31Z

Details

If a parameter of a @wire decorator contains an imported reference, this adds support for tracking of such metadata.
For example @wire(getRecord, { recordId: id }), where userId
is imported: import id from '@salesforce/user/id';
static 1st order references to a literal constant are now resolved, e.g.
```
const foo = '123';
@wire(getRecord, { parameter: foo });
```
In addition to strings, adds support for booleans and numbers
If a parameter is unresolved, adds it to metadata with type 'unresolved'
Adds type to resolved references/expressions
If an array contains an unresolved identifier/expression, marks the array as unresolved
Updates to test suite and various bug fixes

Does this PR introduce a breaking change?

Yes
No

For example `@wire(getRecord, recordId=userId)`, where recordIs is imported: `import userId from '@salesforce/user/id'`;

apapko

Minor changes to handle missing imports

apapko · 2018-09-11T17:29:59Z

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

+        import { wire } from 'lwc';
+        import { getRecord } from 'recordDataService';
+        import userId from '@salesforce/user/Id';
+        export default class Test {


we should test relative import

import rel from './relative'; import rel2 from './other/relative2.js';

I would love to have other tests a well:

const userId = '00xxxxxxxxxxxxxx'; export default class Test { @wire(getRecord, { recordId: userId }) recordData; }

const data = { userId: '00xxxxxxxxxxxxxx' } export default class Test { @wire(getRecord, { recordId: data.userId }) recordData; }

@apapko done
@pmdartus done

apapko · 2018-09-11T17:58:14Z

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


        if (config) {
            wiredValue.static = getWiredStatic(config);
            wiredValue.params = getWiredParams(t, config);
        }

+        const getReferenceByName = name => path.scope.getBinding(name).path.parentPath.node.source.value;


we should add existence check to the output of getBinding(name) , as the import may not exist

path.scope.getBinding is always present, the existence check is not needed.

However the traversal: parentPath.node.source.value look really dangerous, we should be more defensive there. Here are couple of examples where this traversal would throw:
http://astexplorer.net/#/gist/0473ef83527151ddf39e4de17b9f7fab/63c21ccaf6616f56cb93a5a3ce6b3e1e979926ab

looks like path.scope.getBinding is null when import is missing, fixed & added test, also added some defensive checks but I'm not sure whether that's needed for module imports since I added a restriction to only resolve module imports, I'll need to revisit this.

apapko · 2018-09-11T17:59:18Z

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

        if (id.isIdentifier()) {
            wiredValue.adapter = {
                name: id.node.name,
-                reference: path.scope.getBinding(id.node.name).path.parentPath.node.source.value
+                reference: getReferenceByName(id.node.name)


we could preserve id.node.name reference since its accessed twice here.

apapko · 2018-09-11T18:03:39Z

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

    properties.forEach(s => {
        if (s.key.type === 'Identifier' && s.value.type === 'ArrayExpression') {
            ret[s.key.name] = s.value.elements.map(e => e.value);
+        } else if (s.key.type === 'Identifier' && s.value.type === 'Identifier') {
+            ret[s.key.name] = {reference: getReferenceByName(s.value.name)};


should we still assign the value if getReferenceByName returned undefined? (in the event of a missing import)

as per conversation, return undefined explicitly, let me know if I misunderstood the conclusion, I can - not set it otherwise

pmdartus · 2018-09-11T18:24:50Z

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


        if (config) {
            wiredValue.static = getWiredStatic(config);
            wiredValue.params = getWiredParams(t, config);
        }

+        const getReferenceByName = name => path.scope.getBinding(name).path.parentPath.node.source.value;


path.scope.getBinding is always present, the existence check is not needed.

However the traversal: parentPath.node.source.value look really dangerous, we should be more defensive there. Here are couple of examples where this traversal would throw:
http://astexplorer.net/#/gist/0473ef83527151ddf39e4de17b9f7fab/63c21ccaf6616f56cb93a5a3ce6b3e1e979926ab

pmdartus · 2018-09-11T18:27:26Z

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

+        import { wire } from 'lwc';
+        import { getRecord } from 'recordDataService';
+        import userId from '@salesforce/user/Id';
+        export default class Test {


I would love to have other tests a well:

const userId = '00xxxxxxxxxxxxxx'; export default class Test { @wire(getRecord, { recordId: userId }) recordData; }

const data = { userId: '00xxxxxxxxxxxxxx' } export default class Test { @wire(getRecord, { recordId: data.userId }) recordData; }

caridy · 2018-09-11T19:15:09Z

I'm fine with this concept, the only thing to keep clear is that this work for global value providers, and should not be considered for any contextual information since such information can't be imported.

salesforce-best-lwc-internal · 2018-09-11T23:03:07Z

Benchmark results

Base commit: 5634840 | Target commit: d055683

salesforce-best-lwc-internal · 2018-09-11T23:04:38Z

Benchmark results

Base commit: 5634840 | Target commit: 4e0abbf

salesforce-best-lwc-internal · 2018-09-11T23:05:28Z

Benchmark results

Base commit: 5634840 | Target commit: 875004e

kevinv11n

I'm concerned with how the wire service will evaluate { reference: '@salesforce/user/Id' } when it's evaluating the config object to pass to the wire adapter. I need to debug through this with you to ensure all the dots align.

Edit:
I just realized this is for compiler metadata, not the component output itself, right? If so then my concern is greatly reduced and remains only on the comments I've left elsewhere.

kevinv11n · 2018-09-12T04:13:06Z

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

+        `
+        import { wire } from 'lwc';
+        import { getRecord } from 'recordDataService';
+        import { id as currentUserId } from '@salesforce/user/Id';


This is aliasing a named export. All the @salesforce scoped imports use only default exports to handle referential integrity.

@kevinv11n do you proposed this reference to be collected as 'undefined' or not collected at all?

kevinv11n · 2018-09-12T04:15:37Z

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

+        },
+    );
+    pluginTest(
+        'sets reference to undefined for const, let references',


why undefined for const with a static value?

the use case was deemed as unsupported for now (to my understanding)

apapko

Few minor changes.
We briefly spoke about types today when attempting to resolve a complex objects and I wanted to bring up a discussion about the shape of the property value we return. The existing shape of the collected values varies depending on the type of the value we are resolving. For example, we will either return a string or an object depending on the property type. If it is a static 'const' we return a string, however, if it is a module import, we return an object with a reference key. ex:

static: { recordId: { reference: '@salesforce/user/Id' } }

or

static: { recordId: '$recordId' }

I propose to normalize the shape of the value and always return a type and a value. This will allow us to add more details about what exactly has or has not been resolved. For example:

static string:

static: { recordId: {
    type: 'string',
    value: '005000000000000000'
} }

import from module:

static: { recordId: {
    type: 'module',
    value: '@salesforce/userId/id'
} }

import from relative path:

static: { recordId: {
    type: module,
    value: undefined
} }

reference to an object:

params: { recordId: {
    type: 'object',
    value: undefined
} }

let reference

static: { recordId: {
    type: undefined,
    value: undfeined
} }

We could go even further and add something that @pmdartus has proposed, adding 'unresolved' type. This special type can be used to differentiate between undefined values and a failure to resolve a value or a module.

This is discussion is up for debate, but i wanted to get your thoughts on the suggestion.

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

apapko · 2018-09-11T23:12:57Z

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

+                            adapter: { name: 'getRecord', reference: 'recordDataService' },
+                            name: 'recordData',
+                            params: {},
+                            static: {},


should we include the 'dotParameter' and set its value as undefined?

that's what I was asking yesterday, I can add that support but it means adding extra explicit checks for a new value type. I'm ok with it either way, if it's not immediately needed we can always add it later, it's not hard either way.

i say if we decided to spend more time on this PR ( enhancing the value shape ), then it may be worth doing this case as well, otherwise let's move on.

apapko · 2018-09-11T23:32:34Z

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

+        `
+        import { wire } from 'lwc';
+        import { getRecord } from 'recordDataService';
+        const userId = '005000000000000000';


I believe we can support static const? the value of 'let' can be changed, therefore we disallow its use, but the 'const' prevents one from doing that. Although, according to the quip doc, we do not resolve any complex types, therefore the 'const' support can only be done for strings.

to be clear you want us to:

support

const foo = '123'; @wire(.. { foo: foo })

NOT support

const foo = '123; const bar = foo; @wire(... {bar: bar })

Should I do that as part of this story or another ? (the original story was about module imports, but I can tag it to this one).

Yeah, there is a way to walk that up to the original binding, but I'm find with supporting number 1 anyways.

You are right @sfdciuie , It's probably an overkill to keep walking up to resolve the value. We could attempt to do a first level resolution, but not sure if it will worth the effort right now. We can just leave it off as you originally had it then.

ghost · 2018-09-12T16:49:28Z

@kevinv11n yes, this is just the metadata.

ghost · 2018-09-13T00:20:59Z

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

+            wireParameters: ['recordId: data.userId'],
+            expectedStaticParameters: { recordId: { type: 'object' } } });
+
+    wireMetadataParameterTest('when an inline initialisation is used, should use value',


oddly enough, when I added this test it failed, so the metadata wasn't generated for it, had to add code to support it.

nit: we use american spelling so it should be initialization

kevinv11n

Is there support for types other than module, string

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

kevinv11n · 2018-09-13T02:32:46Z

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

+        { wireParameters: ['recordId: id'],
+            expectedStaticParameters: { recordId: {} } });
+
+    wireMetadataParameterTest('when importing with "as" from a module should reference the name of the module',


this is testing as with a named export. can you add a test with as on a default export?

import foo as bar from .. is afaik invalid syntax (since foo is already an alias) ? I just tried and indeed it's flagged as syntax error.
just in case I added a few more cases, so we are testing:

import foo from import {foo as bar} from import { foo } from

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

kevinv11n · 2018-09-13T02:34:35Z

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

+            wireParameters: ['recordId: data.userId'],
+            expectedStaticParameters: { recordId: { type: 'object' } } });
+
+    wireMetadataParameterTest('when an inline initialisation is used, should use value',


nit: we use american spelling so it should be initialization

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

kevinv11n · 2018-09-13T02:35:50Z

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

+    wireMetadataParameterTest('when inline array with a non-string literal is used, should have "undefined" for non-string literal values',
+        { declaration: `const bar = '123';`,
+            wireParameters: ['fields: ["foo", bar]'],
+            expectedStaticParameters: { fields: {type: 'array', value: ['foo', undefined]} } });


i wonder if it's good to do this or treat the whole thing as unresolvable

I was pondering about that, it seems somewhat useful as you'd at least be able to analyze metadata and then figure out that say due to just one parameter an expensive query wasn't pre-cached so action may be taken ?

Up to you though, if you feel that the whole thing should be undefined I can change that.

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

ghost · 2018-09-13T15:10:59Z

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

+                result = {type: 'object', value: undefined};
+            }
+        } else {
+            result = {type: 'unknown', value: `${s.key.type}-${valueType}`};


@apapko @kevinv11n @pmdartus Let me know how you feel about this one, added a new type "unknown" with value containing the key type & value type so that if something pops up we know whether there was a gap or something that we don't plan to support.

Can you give examples where we would return unkown? If it's to guard against cases that we haven't anticipated I would rather fail hard.

pmdartus · 2018-09-13T16:26:54Z

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

+        if (s.key.type === 'Identifier') {
+            if (valueType === 'ArrayExpression') {
+                // @wire(getRecord, { fields: ['Id', 'Name'] })
+                result = {type: 'array', value: s.value.elements.map(e => e.value)};


Do we make sure somewhere else the code that array literal items only contains string literals? If it's the case let's add a comment otherwise, we need to be more defensive when accessing the value property.

see my comment regarding these on the appropriate test, we don't do any extra checks, the references get resolved as "undefined", up to you guys whether you want it to work like this or you want to undefine the whole thing, there are pros and cons with each approach.

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

pmdartus · 2018-09-13T16:32:16Z

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

+                // const userId = '123';
+                // @wire(getRecord, { userId: userId })
+                const reference = getReferenceByName(s.value.name);
+                result = {value: reference.value, type: reference.type};


Let's hardcode identifier in type, the reference.type is equal to Identifier (with a capital I) and will not be consistent with other types.

not sure I understand? the reference.type has nothing to do with babel we set it ourselves in getReferenceByName, it's either going to be 'module' or 'string'/'number'/'boolean' (or undefined)

pmdartus · 2018-09-13T16:32:55Z

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

+                result = {value: reference.value, type: reference.type};
+            } else if (valueType === 'MemberExpression') {
+                // @wire(getRecord, { userId: recordData.Id })
+                result = {type: 'object', value: undefined};


I would rather set the value to null instead of undefined.

I believe the thinking behind this was: If we could resolve the value and we know what it is, then we present that value, if we can't resolve it then it's undefined. Setting it to null might erroneously flag the consuming team into thinking we resolved the expression and it resolved to null
eg.

const data = { foo: null; } @wire (getRecord, { extraParameter: data.foo })

possibly the better solution here to resolve ambiguities such as that, is to instead make everything that we're not resolving type: 'unresolved' with value being essentially the type (as a hint?)
so it could look like for a member expression:
{type: 'unresolved', value: 'member_expression'}
for a reference (ie. Identifier/Identifier)
{type: 'unresolved', value: 'reference'}
the only downside is that value has a special meaning here, which is not symmetrical to the resolved cases
@apapko thoughts ?

pmdartus · 2018-09-13T16:34:55Z

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

+                result = {type: 'object', value: undefined};
+            }
+        } else {
+            result = {type: 'unknown', value: `${s.key.type}-${valueType}`};


Can you give examples where we would return unkown? If it's to guard against cases that we haven't anticipated I would rather fail hard.

pmdartus · 2018-09-13T16:37:55Z

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


        if (config) {
            wiredValue.static = getWiredStatic(config);
            wiredValue.params = getWiredParams(t, config);
        }

+        const getReferenceByName = getScopedReferenceByName.bind(this, path.scope);


bind usage is not intuitive. I would rather use currying:

const referenceLookup = scope => name => { // logic }

I was just going by the existing convention in this file, where everythin was function() {}, I can change that of course,

do you mean

const getScopedReferenceByName = scope => name => { const binding = scope.getBinding(name);

on the outside, and then

const getReferenceByName = getScopedReferenceByName(path.scope);

inside the module.export ?

pmdartus · 2018-09-13T16:38:47Z

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

@@ -94,9 +127,39 @@ function getWiredParamMetadata(properties) {
    return ret;
 }

+function getScopedReferenceByName(scope, name) {
+    let binding = scope.getBinding(name);


Make binding const

apapko

Thank you for adding such great test coverage!

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

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

ghost · 2018-09-14T17:49:37Z

@apapko @kevinv11n @pmdartus Added a final change to mark arrays with unsupported literals as unresolved, and 2 more tests for the cases where one element is unresolved and for the case where all elements are resolved.

Iurie added 2 commits September 11, 2018 13:11

feat: adds metadata support for imported properties in @wire decorator

eedcd8e

For example `@wire(getRecord, recordId=userId)`, where recordIs is imported: `import userId from '@salesforce/user/id'`;

refactor: update function name

378de88

apapko requested changes Sep 11, 2018

View reviewed changes

apapko requested a review from pmdartus September 11, 2018 18:04

pmdartus requested changes Sep 11, 2018

View reviewed changes

Iurie added 3 commits September 11, 2018 18:31

refactor: review feedback

d055683

refactor: review feedback

875004e

refactor: review feedback

4e0abbf

kevinv11n suggested changes Sep 12, 2018

View reviewed changes

kevinv11n reviewed Sep 12, 2018

View reviewed changes

apapko requested changes Sep 12, 2018

View reviewed changes

refactor: more tests, code cleanup, resolve const

94b150b

ghost commented Sep 13, 2018

View reviewed changes

kevinv11n approved these changes Sep 13, 2018

View reviewed changes

refactor: support for numbers, booleans, more tests

d41f3a1

ghost commented Sep 13, 2018

View reviewed changes

fix: hit more conditions

a5cfd96

pmdartus requested changes Sep 13, 2018

View reviewed changes

Iurie added 3 commits September 13, 2018 14:01

fix: some review feedback

890938d

fix: some review feedback

f4ddcbf

refactor: add type 'unresolved'

8d8490e

pmdartus approved these changes Sep 14, 2018

View reviewed changes

apapko approved these changes Sep 14, 2018

View reviewed changes

packages/babel-plugin-transform-lwc-class/src/__tests__/wire-decorator.spec.js Outdated Show resolved Hide resolved

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

fix: test names

ae4b1e0

feat: mark arrays that contain non-supported literals as unresolved

25f7dfa

fix: update test spec

065d9cd

ghost changed the title ~~feat: adds metadata support for imported properties in @wire decorator~~ feat: augments metadata support gathering for @wire decorator Sep 14, 2018

ghost changed the title ~~feat: augments metadata support gathering for @wire decorator~~ feat: augments metadata gathering for @wire decorator Sep 14, 2018

ghost merged commit 4920a4e into salesforce:master Sep 14, 2018

ghost deleted the static-property-reference branch September 14, 2018 18:49

This pull request was closed.

feat: augments metadata gathering for @wire decorator #631

feat: augments metadata gathering for @wire decorator #631

Conversation

ghost commented Sep 11, 2018 • edited by ghost

Details

Does this PR introduce a breaking change?

apapko 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 11, 2018 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy commented Sep 11, 2018

salesforce-best-lwc-internal bot commented Sep 11, 2018

Benchmark results

salesforce-best-lwc-internal bot commented Sep 11, 2018

Benchmark results

salesforce-best-lwc-internal bot commented Sep 11, 2018

Benchmark results

kevinv11n left a comment • edited

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

ghost Sep 12, 2018 • edited by ghost

Choose a reason for hiding this comment

apapko left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 12, 2018 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 12, 2018 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 12, 2018

Choose a reason for hiding this 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

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

ghost Sep 13, 2018 • edited by ghost

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

ghost Sep 13, 2018 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapko left a comment

Choose a reason for hiding this comment

ghost commented Sep 14, 2018

ghost commented Sep 11, 2018 •

edited by ghost

ghost Sep 11, 2018 •

edited by ghost

kevinv11n left a comment •

edited

ghost Sep 12, 2018 •

edited by ghost

apapko left a comment •

edited

ghost Sep 12, 2018 •

edited by ghost

ghost Sep 12, 2018 •

edited by ghost

ghost Sep 13, 2018 •

edited by ghost

ghost Sep 13, 2018 •

edited by ghost