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

fix(babel-plugin-component): correct class member decorator validation #1122

Merged
merged 4 commits into from
Mar 20, 2019
Merged

fix(babel-plugin-component): correct class member decorator validation #1122

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf2bfa7
fix(babel-plugin-component): improve class member decorator validation
apapko Mar 20, 2019
9d060fc
wip: change decorator setter/getter condition
apapko Mar 20, 2019
4e8b083
test: correct test code
apapko Mar 20, 2019
ca7f7b7
wip: remove commented comment
apapko Mar 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions packages/@lwc/babel-plugin-component/src/__tests__/api-decorator.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,121 @@ describe('Transform property', () => {
}
);

pluginTest(
'single @api getter on one class member should not conflict with @api setter on another',
`
import { api } from 'lwc';
export default class Test {
@api
get first() { return null; }

get second() {
return this.s;
}
@api
set second(value) {
this.s = value;
}
}
`,
{
output: {
code: `
import { registerDecorators as _registerDecorators } from "lwc";
import _tmpl from "./test.html";
import { registerComponent as _registerComponent } from "lwc";

class Test {
get first() {
return null;
}

get second() {
return this.s;
}

set second(value) {
this.s = value;
}
}

_registerDecorators(Test, {
publicProps: {
first: {
config: 1
},
second: {
config: 3
}
}
});

export default _registerComponent(Test, {
tmpl: _tmpl
});
`,
},
}
);

pluginTest(
'@api setter on one class member should not conflict with @api getter on another',
`
import { api } from 'lwc';
export default class Test {
@api
set first(value) {}
get first() {}

@api
get second() {
return this.s;
}
set second (value) {
this.s = value;
}
}
`,
{
output: {
code: `
import { registerDecorators as _registerDecorators } from "lwc";
import _tmpl from "./test.html";
import { registerComponent as _registerComponent } from "lwc";

class Test {
set first(value) {}

get first() {}

get second() {
return this.s;
}

set second(value) {
this.s = value;
}
}

_registerDecorators(Test, {
publicProps: {
first: {
config: 3
},
second: {
config: 3
}
}
});

export default _registerComponent(Test, {
tmpl: _tmpl
});
`,
},
}
);

pluginTest(
'transform pairs of setter and getter',
`
Expand Down
39 changes: 22 additions & 17 deletions packages/@lwc/babel-plugin-component/src/decorators/api/validate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,26 +81,30 @@ function validatePropertyName(property) {
}

function validateSingleApiDecoratorOnSetterGetterPair(decorators) {
decorators
.filter(decorator => isApiDecorator(decorator) && decorator.type === DECORATOR_TYPES.SETTER)
.forEach(({ path }) => {
const parentPath = path.parentPath;
const name = parentPath.get('key.name').node;

const associatedGetter = decorators.find(
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain how this find operation was picking up class methods that were not decorated with @api?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it was not. The filter logic resulted in a list of @api decorated setters and the find function was only looking at @api decorated getters. The issue was that we weren't cross referencing setter method name with the getter method name ( line 94 didn't check the current getter method name ).

decorator =>
isApiDecorator(decorator) &&
decorator.type === DECORATOR_TYPES.GETTER &&
parentPath.get('key.name').node === name
);

if (associatedGetter) {
throw generateError(parentPath, {
// keep track of visited class methods
const visitedMethods = new Set();

decorators.forEach(decorator => {
const { path, type } = decorator;

// since we are validating get/set we only look at @api methods
if (
isApiDecorator(decorator) &&
(type === DECORATOR_TYPES.GETTER || type === DECORATOR_TYPES.SETTER)
) {
const methodPath = path.parentPath;
const methodName = methodPath.get('key.name').node;

if (visitedMethods.has(methodName)) {
throw generateError(methodPath, {
errorInfo: DecoratorErrors.SINGLE_DECORATOR_ON_SETTER_GETTER_PAIR,
messageArgs: [name],
messageArgs: [methodName],
});
}
});

visitedMethods.add(methodName);
}
});
}

function validateUniqueness(decorators) {
Expand Down Expand Up @@ -145,6 +149,7 @@ module.exports = function validate(klass, decorators) {
}
});

//console.log('---> decorators: ', decorators);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//console.log('---> decorators: ', decorators);

validateSingleApiDecoratorOnSetterGetterPair(decorators);
validateUniqueness(decorators);
};