-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add themeRequestError reducer and getThemeRequestError selector and tests. #9791
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
For our use case we could store a boolean instead of the actual error object, since I don't think we ever make use of it. But I don't see any harm in storing the error.
} ), | ||
[ THEME_REQUEST_SUCCESS ]: ( state, { siteId, themeId } ) => ( { | ||
...state, | ||
[ siteId ]: Object.assign( {}, state[ siteId ], { [ themeId ]: null } ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively:
[ siteId ]: omit( state[ siteId ], [ themeId ] ),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the brackets around themeId
, I think, tho...
* @return {Object} error object if present or null otherwise | ||
*/ | ||
export function getThemeRequestsError( state, themeId, siteId ) { | ||
const themes = get( state.themes.themeRequestsError, siteId, null ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name is misleading here since we're getting an error, not a themes
array.
I think the whole selector could be shortened to
return get( state.themes.themeRequestsError, [ siteId, themeId ], null );
thanks to the way get
works. (You could consider also dropping the fallback null
-- it will then just return undefined
, which should be expressive enough too, I think.)
export const themeRequestsError = createReducer( {}, { | ||
[ THEME_REQUEST_FAILURE ]: ( state, { siteId, themeId, error } ) => ( { | ||
...state, | ||
[ siteId ]: Object.assign( {}, state[ siteId ], { [ themeId ]: { error } } ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will produce a themeRequestsError
state like this (stolen from your unit tests 😛 ):
2916284: {
vivaro: {
error: 'Request error'
}
}
Let's drop the error layer since it bears no extra information:
2916284: {
vivaro: 'Request error'
}
i.e. just omit the curly braces around error so it doesn't produce an object:
[ siteId ]: Object.assign( {}, state[ siteId ], { [ themeId ]: error } )
which I think you can then write using destructuring as
[ siteId ]: {
...state[ siteId ],
[ themeId ]: error
}
* @param {Object} action Action payload | ||
* @return {Object} Updated state | ||
*/ | ||
export const themeRequestsError = createReducer( {}, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming nitpick: themeRequestErrors
(since our other reducers are plural, too; the Request
middle part should be singular, tho, I think 😉 ).
Left a couple of notes. Good to merge after you've addressed them. Love your unit tests, those are really helpful when reviewing and reasoning about state shape. |
type: THEME_REQUEST_SUCCESS, | ||
siteId: 2916284, | ||
themeId: 841 | ||
themeId: 'twentysixteen' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Good spot.
Info
The new themes Redux sub-tree requires error handling. This PR adds reducer and selector that that respond to theme request action success or error.
Testing
Not connected yet. Will be integrated in #9750,