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

mapStateToProps and mapDispatchToProps looks to me like a viewModel #321

Closed
vladap opened this issue Mar 14, 2016 · 3 comments
Closed

mapStateToProps and mapDispatchToProps looks to me like a viewModel #321

vladap opened this issue Mar 14, 2016 · 3 comments

Comments

@vladap
Copy link

vladap commented Mar 14, 2016

ViewModel maps model (state) into props for rendering and can include commands. It might be benefitial to keep established terminology. It might as well simplify API into a single function. I assume that viewModels are small hence the separation into two function is a boilerplate which could be reduced:

const viewModel = (state, dispatch) {
todos: state.todos,
onTodoClick: id => dispatch({...})
}

const TodoList = connect(viewModel)(TodoList)

@Ciantic
Copy link

Ciantic commented Mar 17, 2016

I don't think they can be combined.

Would you remap the actions to their same properties each time the state changes also?

mapStateToProps and mapDispatchToProps are separate for a good reason, consider the performance: mapStateToProps is actually run several times when state changes, and mapDispatchToProps once (or way fewer anyway than mapStateToProps) it doesn't depend on the state.

I won't start to fight about naming however.

@gaearon
Copy link
Contributor

gaearon commented Mar 17, 2016

Yeah, the separation exists for performance reasons:

mapStateToProps and mapDispatchToProps are separate for a good reason, consider the performance: mapStateToProps is actually run several times when state changes, and mapDispatchToProps once (or way fewer anyway than mapStateToProps) it doesn't depend on the state.

Please see #1 for a history of how this API came to be. The single function approach was considered before and rejected.

@gaearon gaearon closed this as completed Mar 17, 2016
@vladap
Copy link
Author

vladap commented Mar 17, 2016

Makes sense. Naming seems fine to me then.

foiseworth pushed a commit to foiseworth/react-redux that referenced this issue Jul 30, 2016
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

No branches or pull requests

3 participants