-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add an option to allow retun value of bindActionCreators call #57
base: master
Are you sure you want to change the base?
Add an option to allow retun value of bindActionCreators call #57
Conversation
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.
Thank you for your PR @dentuzhik !
Please add few more test cases. Otherwise LGTM 👍
@@ -0,0 +1,16 @@ | |||
# EditorConfig helps developers define and maintain consistent |
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.
❤️❤️❤️ Thank you!!!
@@ -93,3 +93,95 @@ ruleTester.run('mapDispatchToProps-returns-object', rule, { | |||
], | |||
}], | |||
}); | |||
|
|||
ruleTester.run('mapDispatchToProps-returns-object-allowReturnBindFn', rule, { | |||
valid: [ |
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.
Thank you so much for this. Would you please add some more negative cases.
For example when the option is set to true but name of the function is not bindActionCreators
and when the option is set to false.
@@ -0,0 +1,16 @@ | |||
# EditorConfig helps developers define and maintain consistent |
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.
you can add https://npmjs.com/eccheck to CI to verify this, so it doesn't deviate
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.
Sorry, link above triggers 404 for me, is it some private package?
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.
oops sorry, https://www.npmjs.com/package/eclint :-)
specifically, eclint check $(git ls-files)
, added in a prelint
or postlint
script.
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's quite a nice package, have not been aware of this, thanks!
I will play around and see if I can add it in the same PR
is this still in consideration? |
Added a new option
allowReturnBindFn
which does not fail thereact-redux/mapDispatchToProps-returns-object
if result ofmapmapDispatchToProps
is a proxy-call tobindActionCreators
For now we are using it in our fork, but considering some other issues in this repo, it might be a good addition.
Fixes #30