RobotsAndPencils / react-gantry

R&P's React Starter Kit
4 stars 1 forks source link

Proposed Thunks Revisions #27

Closed akrigline closed 6 years ago

akrigline commented 6 years ago

bindActionCreators removed

Its use case is pretty niche apparently and in this case in particular we can avoid it by simply giving mapDispatchToProps an object instead of a function.

The only use case for bindActionCreators is when you want to pass some action creators down to a component that isn't aware of Redux, and you don't want to pass dispatch or the Redux store to it.

Default Export Actions

Switched to a default export for actionCreators. I think this is an opinion thing that we as a group will need to settle on. I personally think this is easier to follow and maintain than import * syntax.

Container Restructure

~Per this discussion around binding, there are a few things we have in our container that are not ideal.~

~Two things we're not supposed to do are:~

~As such I created getProfileDetails() and bound it within the constructor.~

I recind this based on this article. Inline function binds this perfectly fine for this use case.

akrigline commented 6 years ago

Looking more at the 'optimization' side of things, I think the stuff I read was a bit over-reactive.

This guy: https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578 Has some solid points against doing what I did with constructor here. Most notably that this is a premature optimization. While I like the sentiment that you can always go back and optimize, I'm pretty sure none of the projects I've ever been on have actually done that. So I'm pretty 50-50 here on this now.