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

remove options parameter #1528

Closed
wants to merge 1 commit into from

Conversation

phapdinh
Copy link
Contributor

I checked the use of the wrapMapToPropsConstant function in the codebase and it is only used in wrapMapToProps and mapDispatchToProps. All three places, the function passed in doesn't take in a second parameter. This might be left over code from a previous refactor or maybe left over for some future feature that wants to pass options as a parameter into the function. However, I thought I bring it up to your attention.

@netlify
Copy link

netlify bot commented Feb 25, 2020

Deploy preview for react-redux-docs ready!

Built with commit 38dae32

https://deploy-preview-1528--react-redux-docs.netlify.com

@timdorr
Copy link
Member

timdorr commented Feb 25, 2020

Nope, it's passed right here:

const mapStateToProps = initMapStateToProps(dispatch, options)
const mapDispatchToProps = initMapDispatchToProps(dispatch, options)
const mergeProps = initMergeProps(dispatch, options)

@timdorr timdorr closed this Feb 25, 2020
@phapdinh
Copy link
Contributor Author

@timdorr options is passed into the initMapDispatchToProps function, but the getConstant function doesn't use it. So your passing in an option parameter that is not being used in the codebase. I can update the PR to remove the options from the initMapDispatchToProps. However, the options is not being used by the nested function. This would be more apparent in typescript, but right now javascript doesn't throw an error if you pass in unnecessary parameters

@timdorr
Copy link
Member

timdorr commented Feb 25, 2020

Well, the long term plan is to drop connectAdvanced (#1236) and refactor all of these complex factory functions into something much simpler. So, this will go away one way or another. Thanks for bringing it up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants