search

mattkrick / redux-optimistic-ui

a reducer enhancer to enable type-agnostic optimistic updates
MIT License
693 stars 36 forks source link

Does not support nested combineReducers #19

Closed maxlapides closed 7 years ago

maxlapides commented 7 years ago

Hey there! Thanks so much for building this tool, it's super useful!

We're running into an issue where we have nested combineReducers like:

combineReducers({
    section1: combineReducers({
        topics: optimistic(topics),
        details
    }),
    section2: combineReducers({
        categories: optimistic(categories),
        geographies
    })
})

Unfortunately, it looks like redux-optimistic-ui does not support this kind of thing. The REVERT action will only work for section1. Have you run into this before? Thanks so much!

mattkrick commented 7 years ago

Never run into that, happy to fix it if you can find a suitable PR. On the other hand, why would you ever combineReducers below the root reducer? You're just introducing unnecessary closures.

On Feb 9, 2017 1:28 PM, "Max Lapides" notifications@github.com wrote:

Hey there! Thanks so much for building this tool, it's super useful!

We're running into an issue where we have nested combineReducers like:

combineReducers({ combineReducers({ topics: optimistic(topics), details }), combineReducers({ categories: optimistic(categories), geographies }) })

Unfortunately, it looks like redux-optimistic-ui does not support this kind of thing. Have you run into this before? Thanks so much!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mattkrick/redux-optimistic-ui/issues/19, or mute the thread https://github.com/notifications/unsubscribe-auth/AFQjv27kTeJs0GwDJAFbL_hOY1Z8GC9Uks5ra4TigaJpZM4L8sBq .

maxlapides commented 7 years ago

@mattkrick Thanks! Regarding the "why," I got this idea from the Redux docs (http://redux.js.org/docs/api/combineReducers.html):

You may call combineReducers at any level of the reducer hierarchy. It doesn't have to happen at the top. In fact you may use it again to split the child reducers that get too complicated into independent grandchildren, and so on.

We're using this nested structure to easily separate different routes in our application.

I found a quick fix, and submitted a PR for it (https://github.com/mattkrick/redux-optimistic-ui/pull/20). What do you think?

mattkrick commented 7 years ago

fixed in v1.0.0 (that's right, 1.0.0. this is now production ready thanks to @maxlapides!)