Skip to content

Conversation

@mattcgenui
Copy link
Contributor

@mattcgenui mattcgenui commented Mar 21, 2022

No description provided.

@mattcgenui mattcgenui closed this Mar 22, 2022
@mattcgenui mattcgenui reopened this Mar 22, 2022
@mattcgenui mattcgenui marked this pull request as ready for review March 22, 2022 17:39
@shanebdavis
Copy link
Member

I could be wrong, but I think there is an easier way to do this. Reducers can be chained. If the reducer passed in is a function, we can just use logic something like this:

// (initialReducer = {}, ...args) =>
const combinedReducer = combineReducers(reducers)
const overallReducer = (state, action) => initialReducer(combinedReducer(state, action), action)
store.replaceReducer(overallReducer)

@mattcgenui
Copy link
Contributor Author

mattcgenui commented Mar 22, 2022

This should work with a regular reducer function. However, if the initialReducer is a combined reducer, this will throw a UnexpectedStateShapeWarningMessage since we send a state with extra slices. That why we added a workaround if initialReducer.name === 'combination'. Does that make sense?

@shanebdavis
Copy link
Member

Interesting. I didn't know about UnexpectedStateShapeWarning. It's unfortunate to have to end-run a false warning message with some fairly runtime heavy code.

Looking at your overall implementation, I'm concerned about the performance of the rootReducers. They do a lot of work every time an action is dispatched that could be cached. The actual work in the rootReducer should be as light as possible.

Example:

// these never change, but currently they will be executed for every dispatched action
const initialSlices = initialReducer ? initialReducer({}, '') : {}
const initialKeys = Object.keys(initialSlices)
const knownKeys = Object.keys(reducers)

Are there any other opportunities to move any for loops, Object.keys calls or calls like hasOwnProperty out of the run-every-dispatched-action loop of the rootReducer?

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