refactor(redux): Reorganize actions and reducers #212
Conversation
| const actionTypes = ['TOGGLE_DRAWER']; | ||
|
|
||
| const actions = createActions(...actionTypes, { prefix }); | ||
| export default actions; |
There was a problem hiding this comment.
This is a standard way to create both action types (string constants that reducers look for) and their corresponding boilerplate action creators. Almost all sync action creators have a sort of identity signature:
- It returns an action object
- It defines a a
payloadprop on that object, whose value is the first argument - If
payloadis anError, it defines anerrorprop, whose value istrue
The docs suggest that exporting action types and action creators can reduce boilerplate and keep action logic centralized. Here, the action creators generated by createActions are named such that they can act as action types.
actions.toggleDrawer.toString(); // => 'APP/TOGGLE_DRAWER'
actions.toggleDrawer(null); // => { type: 'APP/TOGGLE_DRAWER, payload: null }
| } catch (error) { | ||
| console.log(error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Async action creators aren't actually action creators at all. They don't return an action object with a type, and they aren't handled by reducers. They're just higher-order functions useful for orchestrating dispatches.
Here, loadReducers doesn't even dispatch any actions. Should we still call it an async action creator? or a helper? Is there a difference, and does it matter?
There was a problem hiding this comment.
I think loading reducers should dispatch an action! APP/REDUCERS_ADDED seems like an opportunity for a UI to re-render itself if it knows it's subject to a lot of new additions.
There was a problem hiding this comment.
When a reducer is added, the store actually replaces its root reducer, which instructs connected components to re-render (like an action does). And there's nothing that a reducer should do in response to REDUCERS_ADDED; even updating the list of current slices is pointless, since getState() would be the source of truth for that info.
I think we can improve the reducer-adding workflow—turn it into a pipeline, maybe—as we open up to extensibility. But for now I think this is actually fine.
|
|
||
| const { request } = RestApi.Magento2; | ||
| const storage = new BrowserPersistence(); |
There was a problem hiding this comment.
Since actions are similar to events, a reducer can seem like a good place to "listen" for "events" and perform side effects such as local persistence. However, one of the few firm rules in Redux says that reducers should perform no side effects.
Things you should never do inside a reducer:
- Mutate its arguments;
- Perform side effects like API calls and routing transitions;
- Call non-pure functions, e.g.
Date.now()orMath.random().
So I had to migrate the storage logic from the cart reducer here to the cart actions. It's largely unchanged. However, an alternate way to handle this logic on the "listening" side would be to put it in a middleware.
There was a problem hiding this comment.
BrowserPersistence is a hack in the first place; we need localStorage just so guest carts can survive hard refreshes. Thanks for moving it (assuming it still works and you get the same cart on a hard refresh; do you?), but it's still here temporarily. Logic like this should be in a ServiceWorker, or in a middleware that helps us simulate one.
| await dispatch(createGuestCart()); | ||
| } | ||
|
|
||
| // retrieve app state again | ||
| // retrieve the new guest cart from state | ||
| return getState().cart.guestCartId; | ||
| } |
There was a problem hiding this comment.
Here, we have another example of the ambiguity around async action creators.
- An async action creator is a function that receives a payload and returns a thunk
- A thunk is a function that receives
dispatchandgetState, performs side effects, and returnsundefined
Here, we have a function that's essentially a naked thunk—it's not the product of a higher-order function—and it both performs side effects (using dispatch) and returns a value (using getState). Currently, it's exclusively used by async action creators, since it's of little use to components due to its shape.
What should we do with this function? It feels like an anti-pattern.
| }; | ||
|
|
||
| const enterSubflow = () => | ||
| const actions = createActions(actionMap, ...actionTypes, { prefix }); | ||
| export default actions; |
There was a problem hiding this comment.
createActions, provided by redux-actions, has a wonderful, optional API that allows us to nest action creators. While it's a bit overkill for most action types, I think it's perfect for action types associated with API requests, which often fit a request-success-error pattern.
There was a problem hiding this comment.
Yeah, let's go with prior art here. There should be simple recipes for the sequence of action dispatch API calls and other common interactions (say, additional logic loaded via a chunk?), like:
import { lifecycle } from '@magento/peregrine';
const cartLifecycle = lifecycle('CART');
const cartApiLifecycle = cartLifecycle.requests({
GET_DETAILS(state) {
return {
path: `/guest-carts/${state.guestCartId}`
}
},
ADD_ITEM(state, item) {
return {
path: `/guest-carts/${state.guestCartId}/items/`,
method: 'POST',
body: item
}
},
async (path, method, body) => M2ApiRequest.request(path, { method, body })
);
const [getDetails, addItem] = cartApiLifecycle.getActions();
const reducer = cartApiLifecycle.getReducer();We don't need to make those abstractions yet, of course.
| editing: null, | ||
| step: 'cart', | ||
| submitting: false, | ||
| valid: false | ||
| }; |
There was a problem hiding this comment.
Storing validity here as a simple boolean, valid, is definitely an oversimplification. This will get much more complex as we start to track client- and server-side validation of various parts of the form and the workflow.
There was a problem hiding this comment.
Yeah, I think that reducers should be autogenerated at the same time as actions, see comment above.
| } catch (error) { | ||
| console.log(error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
I think loading reducers should dispatch an action! APP/REDUCERS_ADDED seems like an opportunity for a UI to re-render itself if it knows it's subject to a lot of new additions.
|
|
||
| const { request } = RestApi.Magento2; | ||
| const storage = new BrowserPersistence(); |
There was a problem hiding this comment.
BrowserPersistence is a hack in the first place; we need localStorage just so guest carts can survive hard refreshes. Thanks for moving it (assuming it still works and you get the same cart on a hard refresh; do you?), but it's still here temporarily. Logic like this should be in a ServiceWorker, or in a middleware that helps us simulate one.
| }; | ||
|
|
||
| const enterSubflow = () => | ||
| const actions = createActions(actionMap, ...actionTypes, { prefix }); | ||
| export default actions; |
There was a problem hiding this comment.
Yeah, let's go with prior art here. There should be simple recipes for the sequence of action dispatch API calls and other common interactions (say, additional logic loaded via a chunk?), like:
import { lifecycle } from '@magento/peregrine';
const cartLifecycle = lifecycle('CART');
const cartApiLifecycle = cartLifecycle.requests({
GET_DETAILS(state) {
return {
path: `/guest-carts/${state.guestCartId}`
}
},
ADD_ITEM(state, item) {
return {
path: `/guest-carts/${state.guestCartId}/items/`,
method: 'POST',
body: item
}
},
async (path, method, body) => M2ApiRequest.request(path, { method, body })
);
const [getDetails, addItem] = cartApiLifecycle.getActions();
const reducer = cartApiLifecycle.getReducer();We don't need to make those abstractions yet, of course.
| case 'STEP_3': { | ||
| child = <Exit resetCheckout={resetCheckout} />; | ||
| break; | ||
| case 'receipt': { |
There was a problem hiding this comment.
This seems like a mismatch. Why's it here in this otherwise very consistent flow, which sets child and identifies itself as a step number?
packages/venia-concept/src/index.js
Outdated
| @@ -23,17 +23,4 @@ ReactDOM.render( | |||
| document.getElementById('root') | |||
| ); | |||
|
|
|||
| if (process.env.SERVICE_WORKER && 'serviceWorker' in navigator) { | |||
There was a problem hiding this comment.
This change seems unrelated.
There was a problem hiding this comment.
Good catch. This shouldn't have been included in the PR.
| editing: null, | ||
| step: 'cart', | ||
| submitting: false, | ||
| valid: false | ||
| }; |
There was a problem hiding this comment.
Yeah, I think that reducers should be autogenerated at the same time as actions, see comment above.
jimbo
force-pushed
the
jimbo/redux-reorg
branch
4 times, most recently
from
5df5a8b
to
af9bb01
Compare
jimbo
force-pushed
the
jimbo/redux-reorg
branch
from
b064e4a
to
3eaf116
Compare
This PR is a:
[ ] New feature
[x] Enhancement/Optimization
[x] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation
Summary
When this pull request is merged, we'll once again use
redux-actionsto create reducers and actions. This will prepare us for incoming changes related to extensibility. It will also make them far more consistent.Additional information
Checkout components have been refactored to accommodate new app state structure. They should now be ready for editable forms to be added (and real cart modification requests to be made).