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
14 changes: 14 additions & 0 deletions docs/rules/named-functions-in-promises.md
Expand Up @@ -2,6 +2,20 @@

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

#### Configuration

```
ember/named-functions-in-promises: [2, {
allowSimpleArrowFunction: false,
}]
```

Setting `allowSimpleArrowFunction` to `true` allows arrow function expressions that do not have block bodies.
These simple arrow functions must also only contain a single function call.
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
20 changes: 14 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 allowSimpleArrowFunction = options.allowSimpleArrowFunction || false;

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

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

if (
hasPromiseExpression(node) &&
(
if (hasPromiseExpression(node)) {
if (
allowSimpleArrowFunction &&
utils.isConciseArrowFunctionWithCallExpression(firstArg)
) {
return;
}
if (
utils.isFunctionExpression(firstArg) ||
utils.isArrowFunctionExpression(firstArg)
)
) {
report(node);
) {
report(node);
}
}
},
};
Expand Down
15 changes: 15 additions & 0 deletions lib/utils/utils.js
Expand Up @@ -10,6 +10,7 @@ module.exports = {
isArrayExpression,
isFunctionExpression,
isArrowFunctionExpression,
isConciseArrowFunctionWithCallExpression,
isNewExpression,
isCallWithFunctionExpression,
isThisExpression,
Expand Down Expand Up @@ -117,6 +118,20 @@ function isArrowFunctionExpression(node) {
return node !== undefined && node.type === 'ArrowFunctionExpression';
}

/**
* Check whether or not a node is an ArrowFunctionExpression with concise body
* that contains a call expression.
*
* @param {Object} node The node to check.
* @returns {boolean} Whether or not the node is an ArrowFunctionExpression
* with concise body.
*/
function isConciseArrowFunctionWithCallExpression(node) {
return isArrowFunctionExpression(node) &&
node.expression &&
isCallExpression(node.body);
}

/**
* Check whether or not a node is an NewExpression.
*
Expand Down
82 changes: 80 additions & 2 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: [{
allowSimpleArrowFunction: true,
}],
}, {
code: 'user.save().catch(err => this._handleError(err));',
parserOptions: { ecmaVersion: 6 },
options: [{
allowSimpleArrowFunction: true,
}],
}, {
code: 'user.save().finally(() => this._finallyDo());',
parserOptions: { ecmaVersion: 6 },
options: [{
allowSimpleArrowFunction: 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: [{
allowSimpleArrowFunction: true,
}],
},
],
invalid: [
Expand All @@ -49,23 +73,77 @@ 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().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().then(() => {return user.reload();});',
parserOptions: { ecmaVersion: 6 },
options: [{
allowSimpleArrowFunction: true,
}],
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
}, {
code: 'user.save().catch(() => {return error.handle();});',
parserOptions: { ecmaVersion: 6 },
options: [{
allowSimpleArrowFunction: true,
}],
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
}, {
code: 'user.save().finally(() => {return finallyDo();});',
parserOptions: { ecmaVersion: 6 },
options: [{
allowSimpleArrowFunction: true,
}],
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
},
}, {
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: [{
allowSimpleArrowFunction: true,
}],
errors: [{
message: 'Use named functions defined on objects to handle promises',
}],
}
],
});
8 changes: 8 additions & 0 deletions tests/lib/utils/utils-test.js
Expand Up @@ -94,6 +94,14 @@ describe('isArrowFunctionExpression', () => {
});
});

describe('isConciseArrowFunctionExpressionWithCall', () => {
const node = parse('test = () => foo()').right;

it('should check if node is concise arrow function expression with call expression body', () => {
assert.ok(utils.isConciseArrowFunctionWithCallExpression(node));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot help but mention that test case for is _not* consise arrow function expression would be nice to have - do you think it's redundant?

Copy link
Member

Choose a reason for hiding this comment

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

In this case it might have a sense indeed. Just to be sure that arrow function with body is not filling requirements of this function. Could you please address that as well @sudowork ? :)

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.

Done @bardzusny @michalsnik 😄 . Added two test cases:

  • One for arrow using block body
  • One for concise arrow without a call expression

});

describe('isNewExpression', () => {
const node = parse('new Date()');

Expand Down