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 2 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
34 changes: 32 additions & 2 deletions packages/babel-plugin-transform-lwc-class/src/__tests__/wire-decorator.spec.js
View file
Open in desktop
Expand Up @@ -126,7 +126,7 @@ Test.wire = {
}
};`
}
})
});

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

describe('Metadata', () => {
pluginTest(
'gather track metadata',
'gather wire metadata',
`
import { wire } from 'lwc';
import { getFoo } from 'data-service';
Expand Down Expand Up @@ -367,4 +367,34 @@ describe('Metadata', () => {
},
},
);
pluginTest(
'gather wire metadata for an imported property',
`
import { wire } from 'lwc';
import { getRecord } from 'recordDataService';
import userId from '@salesforce/user/Id';
export default class Test {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should test relative import

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@apapko done
@pmdartus done

@wire(getRecord, { recordId: userId })
recordData;
}
`,
{
output: {
metadata: {
decorators: [{
type: 'wire',
targets: [
{
adapter: { name: 'getRecord', reference: 'recordDataService' },
name: 'recordData',
params: {},
static: { recordId: { reference: '@salesforce/user/Id' } },
type: 'property',
}
],
}]
},
},
},
);
});
16 changes: 10 additions & 6 deletions packages/babel-plugin-transform-lwc-class/src/decorators/wire/transform.js
View file
Open in desktop
Expand Up @@ -74,11 +74,13 @@ function buildWireConfigValue(t, wiredValues) {
}));
}

function getWiredStaticMetadata(properties) {
const ret = {};
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);
} else if (s.key.type === 'Identifier' && s.value.type === 'Identifier') {
ret[s.key.name] = {reference: getReferenceByName(s.value.name)};
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

@ghost ghost Sep 11, 2018 •
edited by ghost

Choose a reason for hiding this comment

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

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

}
This conversation was marked as resolved.
Show resolved Hide resolved
});
return ret;
Expand Down Expand Up @@ -107,17 +109,19 @@ module.exports = function transform(t, klass, decorators) {
const wiredValue = {
propertyName,
isClassMethod
}
};

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.


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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

done

}
}

Expand All @@ -128,7 +132,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 +158,4 @@ module.exports = function transform(t, klass, decorators) {
targets: metadata
};
}
}
};