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.

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

Allow concise ArrowFunctionExpression (named-functions-in-promises) #64

Merged
merged 9 commits into from May 23, 2017
13 changes: 13 additions & 0 deletions docs/rules/named-functions-in-promises.md
Expand Up @@ -2,6 +2,19 @@

### Rule name: `named-functions-in-promises`

#### Configuration

```
ember/named-functions-in-promises: [2, {
allowConciseArrow: false,
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking about renaming this setting to allowSimpleArrowFunction - I think it would be more descriptive. What do you think? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good and would probably make more sense to users. I was trying to reuse the language from MDN regarding "concise body".

}]
```

Setting `allowConciseArrow` to `true` allows arrow function expressions that do not have block bodies.
For example: `.then(user => this._reloadUser(user))`.

#### Description

When you use promises and its handlers, use named functions defined on parent object. Thus, you will be able to test them in isolation using unit tests without any additional mocking.

```javascript
Expand Down
23 changes: 17 additions & 6 deletions lib/rules/named-functions-in-promises.js
Expand Up @@ -12,6 +12,9 @@ module.exports = {
},

create(context) {
const options = context.options[0] || {};
const allowConciseArrow = options.allowConciseArrow || false;

const message = 'Use named functions defined on objects to handle promises';

const report = function (node) {
Expand All @@ -22,14 +25,16 @@ module.exports = {
CallExpression(node) {
const firstArg = node.arguments[0];

if (
hasPromiseExpression(node) &&
(
if (hasPromiseExpression(node)) {
if (allowConciseArrow && isConciseArrowFunctionWithCallExpression(firstArg)) {
return;
}
if (
utils.isFunctionExpression(firstArg) ||
utils.isArrowFunctionExpression(firstArg)
)
) {
report(node);
) {
report(node);
}
}
},
};
Expand All @@ -42,5 +47,11 @@ module.exports = {
utils.isIdentifier(callee.property) &&
promisesMethods.indexOf(callee.property.name) > -1;
}

function isConciseArrowFunctionWithCallExpression(node) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this function to utilities, where other functions are, and write unit test for this specific function as well. What do you think?

Copy link
Contributor Author

@sudowork sudowork May 23, 2017

Choose a reason for hiding this comment

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

Will do! I didn't do this at first because I was a bit worried it was too specific.

return utils.isArrowFunctionExpression(node) &&
node.expression &&
utils.isCallExpression(node.body);
}
}
};
59 changes: 55 additions & 4 deletions tests/lib/rules/named-functions-in-promises.js
Expand Up @@ -39,6 +39,30 @@ eslintTester.run('named-functions-in-promises', rule, {
}, {
code: 'user.save().finally(_finallyDo);',
parserOptions: { ecmaVersion: 6 },
}, {
code: 'user.save().then(() => this._reloadUser(user));',
parserOptions: { ecmaVersion: 6 },
options: [{
allowConciseArrow: true,
}],
}, {
code: 'user.save().catch(err => this._handleError(err));',
parserOptions: { ecmaVersion: 6 },
options: [{
allowConciseArrow: true,
}],
}, {
code: 'user.save().finally(() => this._finallyDo());',
parserOptions: { ecmaVersion: 6 },
options: [{
allowConciseArrow: true,
}],
}, {
code: 'user.save().then(() => user.reload());',
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if I agree with this case. I think you should probably write user => this._reloadUser(user) anyway, to be able to test it nicely. What do you think @bardzusny? I see it was like this before but then it was updated..

Copy link
Member

Choose a reason for hiding this comment

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

I was just speaking with @bardzusny and we agreed that in this PR it's ok to allow using xxx.method() to keep it consistent with current regular function expressions that are allowed. We do however think that it might be worth considering in next PR to add additional optional setting called for example requireThisBinding. This setting in practice would force you to use this.method.bind(this) or () => this.method(). I'll create separate issue with this proposition though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me!

parserOptions: { ecmaVersion: 6 },
options: [{
allowConciseArrow: true,
}],
},
],
invalid: [
Expand All @@ -49,23 +73,50 @@ eslintTester.run('named-functions-in-promises', rule, {
message: 'Use named functions defined on objects to handle promises',
}],
}, {
code: 'user.save().then(() => user.reload());',
code: 'user.save().catch(() => {return error.handle();});',
parserOptions: { ecmaVersion: 6 },
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
}, {
code: 'user.save().catch(() => {return error.handle();});',
code: 'user.save().finally(() => {return finallyDo();});',
Copy link
Member

Choose a reason for hiding this comment

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

I'd add one or three more tests like this, but with allowConciseArrow set to true just to be totally sure it works like expected :) Of course it should fail then too, because of the arrow function's body not being a CallExpression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

parserOptions: { ecmaVersion: 6 },
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
}, {
code: 'user.save().finally(() => {return finallyDo();});',
code: 'user.save().then(() => this._reloadUser(user));',
parserOptions: { ecmaVersion: 6 },
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
},
}, {
code: 'user.save().catch(err => this._handleError(err));',
parserOptions: { ecmaVersion: 6 },
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
}, {
code: 'user.save().finally(() => this._finallyDo());',
parserOptions: { ecmaVersion: 6 },
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
}, {
code: 'user.save().then(() => user.reload());',
parserOptions: { ecmaVersion: 6 },
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
}, {
code: 'user.save().then(user => user.name);',
parserOptions: { ecmaVersion: 6 },
options: [{
allowConciseArrow: true,
}],
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
}
],
});