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.

Sign up for GitHub

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

Jump to bottom

feat: augments metadata gathering for @wire decorator #631

Merged
14 commits merged into from Sep 14, 2018
Merged

feat: augments metadata gathering for @wire decorator #631

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
eedcd8e
feat: adds metadata support for imported properties in @wire decorator
Sep 11, 2018
378de88
refactor: update function name
Sep 11, 2018
d055683
refactor: review feedback
Sep 11, 2018
875004e
refactor: review feedback
Sep 11, 2018
4e0abbf
refactor: review feedback
Sep 11, 2018
94b150b
refactor: more tests, code cleanup, resolve const
Sep 13, 2018
d41f3a1
refactor: support for numbers, booleans, more tests
Sep 13, 2018
a5cfd96
fix: hit more conditions
Sep 13, 2018
890938d
fix: some review feedback
Sep 13, 2018
f4ddcbf
fix: some review feedback
Sep 13, 2018
8d8490e
refactor: add type 'unresolved'
Sep 13, 2018
ae4b1e0
fix: test names
Sep 14, 2018
25f7dfa
feat: mark arrays that contain non-supported literals as unresolved
Sep 14, 2018
065d9cd
fix: update test spec
Sep 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
125 changes: 121 additions & 4 deletions packages/babel-plugin-transform-lwc-class/src/__tests__/wire-decorator.spec.js
View file
Open in desktop
@@ -1,5 +1,39 @@
const pluginTest = require('./utils/test-transform').pluginTest(require('../index'));

const wireMetadataParameterTest =
(testName, {declaration = '', wireParameters = '', expectedStaticParameters = {}, expectedVariableParameters = {}}) => {
pluginTest(
testName,
`
import { wire } from 'lwc';
import { getRecord } from 'recordDataService';
${declaration};
export default class Test {
@wire(getRecord, { ${wireParameters.join(',')} })
recordData;
}
`,
{
output: {
metadata: {
decorators: [{
type: 'wire',
targets: [
{
adapter: { name: 'getRecord', reference: 'recordDataService' },
name: 'recordData',
params: expectedVariableParameters,
static: expectedStaticParameters ,
type: 'property',
}
],
}]
},
},
},
);
};

describe('Transform property', () => {
pluginTest('transforms wired field', `
import { wire } from 'lwc';
Expand Down Expand Up @@ -126,7 +160,7 @@ Test.wire = {
}
};`
}
})
});

pluginTest('decorator accepts an optional config object as second parameter', `
import { wire } from 'lwc';
Expand Down Expand Up @@ -331,7 +365,7 @@ Test.wire = {

describe('Metadata', () => {
pluginTest(
'gather track metadata',
'gather wire metadata',
`
import { wire } from 'lwc';
import { getFoo } from 'data-service';
Expand All @@ -352,19 +386,102 @@ describe('Metadata', () => {
adapter: { name: 'getFoo', reference: 'data-service' },
name: 'wiredProp',
params: { key1: 'prop1' },
static: { key2: ['fixed'] },
static: { key2: { value: ['fixed'], type: 'array' } },
type: 'property',
},
{
adapter: { name: 'getFoo', reference: 'data-service' },
name: 'wiredMethod',
params: { key1: 'prop1' },
static: { key2: ['fixed'] },
static: { key2: { value: ['fixed'], type: 'array' } },
type: 'method',
}],
}]
},
},
},
);

wireMetadataParameterTest('when constant initialised to a string-literal should extract the value',
{ declaration: `const stringConstant = '123';`,
wireParameters: ['userId: stringConstant'],
expectedStaticParameters: { userId: { value: '123', type: 'string'} } });

wireMetadataParameterTest('when constant initialised to a number-literal should extract the value',
{ declaration: `const numberConstant = 100;`,
wireParameters: ['size: numberConstant'],
expectedStaticParameters: { size: { value: 100, type: 'number'} } });

wireMetadataParameterTest('when constant initialised to a boolean-literal should extract the value',
{ declaration: `const booleanConstant = true;`,
wireParameters: ['isRegistered: booleanConstant'],
expectedStaticParameters: { isRegistered: { value: true, type: 'boolean'} } });

wireMetadataParameterTest('when constant initialised as a reference to another should mark as unresolved',
{ declaration: `const stringConstant = '123'; const referenceConstant = stringConstant;`,
wireParameters: ['recordId: referenceConstant'],
expectedStaticParameters: { recordId: {} } });

wireMetadataParameterTest('when importing a default export from a module should reference the name of the module',
{ declaration: `import id from '@salesforce/user/Id';`,
wireParameters: ['recordId: id'],
expectedStaticParameters: { recordId: { value: '@salesforce/user/Id', type: 'module' } } });

wireMetadataParameterTest('when parameter reference missing should mark as undefined',
{ wireParameters: ['recordId: id'],
expectedStaticParameters: { recordId: {} } });

wireMetadataParameterTest('when importing named export with "as" from a module should reference the name of the module',
{ declaration: `import { id as currentUserId } from '@salesforce/user/Id';`,
wireParameters: ['recordId: currentUserId'],
expectedStaticParameters: { recordId: { value: '@salesforce/user/Id', type: 'module' } } });
This conversation was marked as resolved.
Show resolved Hide resolved

wireMetadataParameterTest('when importing a named export from a module should reference the name of the module',
{ declaration: `import { id } from '@salesforce/user/Id';`,
wireParameters: ['recordId: id'],
expectedStaticParameters: { recordId: { value: '@salesforce/user/Id', type: 'module' } } });

wireMetadataParameterTest('when importing from a relative module should reference the name of the module',
{ declaration: `import id from './someReference.js';`,
wireParameters: ['recordId: id'],
expectedStaticParameters: { recordId: { value: './someReference.js', type: 'module' } } });

wireMetadataParameterTest('when referencing a "let" variable should mark as undefined',
{ declaration: `let userId = '123';`,
wireParameters: ['recordId: userId'],
expectedStaticParameters: { recordId: {} } });

wireMetadataParameterTest('when referencing a member expression, should mark as undefined with type object',
This conversation was marked as resolved.
Show resolved Hide resolved
{ declaration: `const data = {userId: '123'};`,
wireParameters: ['recordId: data.userId'],
expectedStaticParameters: { recordId: { type: 'object' } } });

wireMetadataParameterTest('when an inline string-literal initialization is used, should use value',
{ wireParameters: ['recordId: "123"'],
expectedStaticParameters: { recordId: { value: '123', type: 'string' } } });

wireMetadataParameterTest('when an inline numeric-literal initialization is used, should use value',
{ wireParameters: ['size: 100'],
expectedStaticParameters: { size: { value: 100, type: 'number' } } });

wireMetadataParameterTest('when an inline float-literal initialization is used, should use value',
{ wireParameters: ['underPrice: 50.50'],
expectedStaticParameters: { underPrice: { value: 50.50, type: 'number' } } });

wireMetadataParameterTest('when an inline boolean-literal initialization is used, should use value',
{ wireParameters: ['isRegistered: true'],
expectedStaticParameters: { isRegistered: { value: true, type: 'boolean' } } });

wireMetadataParameterTest('when $foo parameters are used, should use name of the parameter',
{ wireParameters: ['recordId: "$userId"'],
expectedVariableParameters: { recordId: 'userId' } });

wireMetadataParameterTest('when $foo parameters with dots are used, should use name of the parameter',
This conversation was marked as resolved.
Show resolved Hide resolved
{ wireParameters: ['recordId: "$userRecord.Id"'],
expectedVariableParameters: { recordId: 'userRecord.Id' } });

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]} } });
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

});
87 changes: 77 additions & 10 deletions packages/babel-plugin-transform-lwc-class/src/decorators/wire/transform.js
View file
Open in desktop
Expand Up @@ -74,12 +74,45 @@ function buildWireConfigValue(t, wiredValues) {
}));
}

function getWiredStaticMetadata(properties) {
const ret = {};
const supportedValueTypeToMetadataType = {
This conversation was marked as resolved.
Show resolved Hide resolved
'StringLiteral': 'string',
This conversation was marked as resolved.
Show resolved Hide resolved
'NumericLiteral': 'number',
'BooleanLiteral': 'boolean'
};

function getWiredStaticMetadata(properties, getReferenceByName) {
const ret = {};
properties.forEach(s => {
if (s.key.type === 'Identifier' && s.value.type === 'ArrayExpression') {
ret[s.key.name] = s.value.elements.map(e => e.value);
let result = {};
const valueType = s.value.type;
if (s.key.type === 'Identifier') {
if (valueType === 'ArrayExpression') {
// @wire(getRecord, { fields: ['Id', 'Name'] })
result = {type: 'array', value: s.value.elements.map(e => e.value)};
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 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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

} else if (supportedValueTypeToMetadataType[valueType]) {
// @wire(getRecord, { companyName: ['Acme'] })
// @wire(getRecord, { size: 100 })
// @wire(getRecord, { isAdmin: true })
result = {type: supportedValueTypeToMetadataType[valueType], value: s.value.value};
} else if (valueType === 'Identifier') {
// References such as:
// 1. Modules
// import id from '@salesforce/user/id'
// @wire(getRecord, { userId: id })
//
// 2. 1st order constant references with string literals
// const userId = '123';
// @wire(getRecord, { userId: userId })
const reference = getReferenceByName(s.value.name);
result = {value: reference.value, type: reference.type};
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@ghost ghost Sep 13, 2018 •
edited by ghost

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

Oh my bad.

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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 })

Copy link
Author

Choose a reason for hiding this comment

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

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 ?

}
} else {
result = {type: 'unknown', value: `${s.key.type}-${valueType}`};
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
This conversation was marked as resolved.
Show resolved Hide resolved
ret[s.key.name] = result;
});
return ret;
}
Expand All @@ -94,9 +127,39 @@ function getWiredParamMetadata(properties) {
return ret;
}

function getScopedReferenceByName(scope, name) {
let binding = scope.getBinding(name);
Copy link
Member

Choose a reason for hiding this comment

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

Make binding const

Copy link
Author

Choose a reason for hiding this comment

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

done


let type;
let value;

if (binding) {
if (binding.kind === 'module') {
// Resolves module import to the name of the module imported
// e.g. import { foo } from 'bar' gives value 'bar' for `name == 'foo'
let parentPathNode = binding.path.parentPath.node;
if (parentPathNode && parentPathNode.source) {
type = 'module';
value = parentPathNode.source.value;
}
} else if (binding.kind === 'const') {
// Resolves `const foo = 'text';` references to value 'text', where `name == 'foo'`
const init = binding.path.node.init;
if (init && supportedValueTypeToMetadataType[init.type]) {
type = supportedValueTypeToMetadataType[init.type];
value = init.value;
}
}
}
return {
type,
value
};
}

module.exports = function transform(t, klass, decorators) {
const metadata = [];
const wiredValues = decorators.filter(isWireDecorator).map(({ path }) => {
const wiredValues = decorators.filter(isWireDecorator).map(({path}) => {
const [id, config] = path.get('arguments');

const propertyName = path.parentPath.get('key.name').node;
Expand All @@ -107,17 +170,21 @@ module.exports = function transform(t, klass, decorators) {
const wiredValue = {
propertyName,
isClassMethod
}
};

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

const getReferenceByName = getScopedReferenceByName.bind(this, path.scope);
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Author

@ghost ghost Sep 13, 2018 •
edited by ghost

Choose a reason for hiding this comment

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

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 ?


if (id.isIdentifier()) {
const adapterName = id.node.name;
const referenceByName = getReferenceByName(adapterName);
wiredValue.adapter = {
name: id.node.name,
reference: path.scope.getBinding(id.node.name).path.parentPath.node.source.value
name: adapterName,
reference: referenceByName.type === 'module' ? referenceByName.value : undefined
}
}

Expand All @@ -128,7 +195,7 @@ module.exports = function transform(t, klass, decorators) {
};

if (config) {
wireMetadata.static = getWiredStaticMetadata(wiredValue.static);
wireMetadata.static = getWiredStaticMetadata(wiredValue.static, getReferenceByName);
wireMetadata.params = getWiredParamMetadata(wiredValue.params);
}

Expand All @@ -154,4 +221,4 @@ module.exports = function transform(t, klass, decorators) {
targets: metadata
};
}
}
};