-
-
Notifications
You must be signed in to change notification settings - Fork 203
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
Allow concise ArrowFunctionExpression (named-functions-in-promises) #64
Conversation
This is to ensure that the promise handler is defined as a method on the parent object and not some other object.
Edit: This is no longer the case as of 4d01fda after some discussion in #63. Made the allowable case even more stringent by ensuring that the // GOOD
user.save()
.then(() => this._reloadUser(user))
.catch(err => this._notifyAboutFailure(err))
// BAD
user.save()
.then(() => user.reload())
.catch(err => this._notifyAboutFailure(err)) While I'm not sure if this is being too strict, I think erring on the side of being more strict is better. This additional check enforces that your promise handler is defined on the parent itself and not calling out to another object. |
@bardzusny I've updated this PR to:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sudowork !
First of all thanks a lot for working on this. It's looking really good :)
Sorry for such late answer but I didn't have much time recently, I left few comments however, check it out and let me know what do you think :)
@@ -42,5 +47,11 @@ module.exports = { | |||
utils.isIdentifier(callee.property) && | |||
promisesMethods.indexOf(callee.property.name) > -1; | |||
} | |||
|
|||
function isConciseArrowFunctionWithCallExpression(node) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
allowConciseArrow: true, | ||
}], | ||
}, { | ||
code: 'user.save().then(() => user.reload());', |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 }, | ||
errors: [{ | ||
message: 'Use named functions defined on objects to handle promises', | ||
}], | ||
}, { | ||
code: 'user.save().catch(() => {return error.handle();});', | ||
code: 'user.save().finally(() => {return finallyDo();});', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
|
||
``` | ||
ember/named-functions-in-promises: [2, { | ||
allowConciseArrow: false, |
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
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".
- Rename `allowConciseArrow` to `allowSimpleArrowFunction` - Update documentation for `allowSimpleArrowFunction` - Move `isConciseArrowFunctionWithCallExpression` to utils - Add unit test for `isConciseArrowFunctionWithCallExpression` - Add invalid test cases for arrow functions with bodies when `allowSimpleArrowFunction` is set to true
@michalsnik Updated to address comments! Thanks for reviewing :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was fast! Looks great IMO 🚀 I'll wait for one more approval and then I'll merge this. Thanks for everything @sudowork :)
tests/lib/utils/utils-test.js
Outdated
|
||
it('should check if node is concise arrow function expression with call expression body', () => { | ||
assert.ok(utils.isConciseArrowFunctionWithCallExpression(node)); | ||
}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ? :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
In relation to #63, this is the change I'm proposing to support binding context using arrow functions while still maintaining while still maintaining testability.
Here's the example for reference:
Note: This change explicitly allows the concise version of an
ArrowFunctionExpression
and does not allow arrow functions with explicit() => { /* function bodies */ }
. In addition, it checks to ensure that the expression body is aCallExpression
to ensure the logic is not happening in the arrow function itself.Fixes #63