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 the need for base Action Creators to be thunks #2895

Merged
merged 9 commits into from
Jan 13, 2018

Conversation

asauber
Copy link
Contributor

@asauber asauber commented Jan 11, 2018

Currently, two of our base action creators are thunks, the action creator for updating "ONE" resource and the action creator for updating "MANY" resources.

The reason that these are thunks are to support dynamic accessor properties on the api endpoint configs. Every time a ONE or MANY action is dispatched the dispatch function is passed as part of the action. The reducer then uses this reference to dispatch to attach a thunk with Object.defineProperty ... get on the objects stored in the Redux store. The only reason that these dynamically generated functions are thunks are so they can "cross-cut" the Redux state at runtime.

This is an anti-pattern for multiple reasons. Functions should not be stored in Redux, because Redux stage should be serializable. Also, the Redux store should be as flat as possible, acting as a database for components to aggregate data and map this data to declarative properties.

Refer to this Redux Issue for an extended discussion.
reduxjs/redux#749

This PR removes this pattern, these dynamic properties, and implements flat state access for linode.type and linode.image, which were the only two use cases.

@ghost ghost assigned asauber Jan 11, 2018
@ghost ghost added the Backlog label Jan 11, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 51.09% when pulling bad2f344a9ef7bf8c4eba5e4c27d8082f118cc18 on asauber:unthunk-action-creators into 3cbb467 on linode:develop.

Copy link
Contributor

@rjstires rjstires left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, and with all the sincerity in the world, excellent work. Well on the way to being teachable and testable.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 47.238% when pulling ae2d361 on asauber:unthunk-action-creators into 84827a4 on linode:develop.

@asauber asauber merged commit afb7aa8 into linode:develop Jan 13, 2018
@ghost ghost removed the Backlog label Jan 13, 2018
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.

3 participants