Feature Request: Allow reducers to consult global state #1768

Closed
wants to merge 8 commits into
from

Conversation

8 participants
@eremzeit

In combineReducers I love the idea of having reducers each map their input state to a specific path and that generally they only are interested in knowing the input state of that same path. However, sometimes there are cases that even inside the reducer that writes state for a specific path that you need to access something from the global state. This would happen if you maintain some global resource cache on the top level of the state (eg. via normalizr) that doesn't need to be duplicated at each key path.

If you are okay with this change I can also update the readme.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 25, 2016

Collaborator

This has been suggested numerous times so we probably should consider it.
I’m wary for the following reasons:

  • It is not obvious that the previous state gets passed. What if you want to have the updated one?
  • It only works one level deep. What if you add an extra layer of combineReducers()?

If you wire things like this by hand right away instead of using combineReducers(), none of these are a problem because you’re in full control, and everything is explicit.

Collaborator

gaearon commented May 25, 2016

This has been suggested numerous times so we probably should consider it.
I’m wary for the following reasons:

  • It is not obvious that the previous state gets passed. What if you want to have the updated one?
  • It only works one level deep. What if you add an extra layer of combineReducers()?

If you wire things like this by hand right away instead of using combineReducers(), none of these are a problem because you’re in full control, and everything is explicit.

@eremzeit

This comment has been minimized.

Show comment
Hide comment
@eremzeit

eremzeit May 25, 2016

I just changed it to include the new state because part of me agrees with you. But now after thinking about it, I'm wondering if only passing the original global state makes sense as a way of enforcing the spirit of paths. In other words, reducers shouldn't really rely on the state changes on other paths caused by previous reducers. I could be sold either way.

Your second point is valid. In my current implementation it's not a "global" state. If you used combineReducers() below the first level, then the third argument passed to a reducer would just hold the state to the next higher level.

If you wanted to pass the true global state to a reducer and still allow for multiple levels, you'd need a version of combineReducers(reducerMap) that allows for multiple levels of reducerMap. Since this function would have access to the entire tree of reducers, it could pass each reducer the global state regardless of high deep it is in the tree. For example, you would call it as such:

combineReducerTree({
  foo : fooReducerFn,
  bar : {
    baz: bazReducerFn,
    buzz: buzzReducerFn
  }
})

Using this would actually be a more declarative way of expressing multiple levels of reducers than just manually applying combineReducers() multiple times. Conveniently, combineReducerTree() would just be a logical superset of combineReducers() so if you so chose you could just replace the function.

If you like this idea I can expand this pull request to implement such a function.

I just changed it to include the new state because part of me agrees with you. But now after thinking about it, I'm wondering if only passing the original global state makes sense as a way of enforcing the spirit of paths. In other words, reducers shouldn't really rely on the state changes on other paths caused by previous reducers. I could be sold either way.

Your second point is valid. In my current implementation it's not a "global" state. If you used combineReducers() below the first level, then the third argument passed to a reducer would just hold the state to the next higher level.

If you wanted to pass the true global state to a reducer and still allow for multiple levels, you'd need a version of combineReducers(reducerMap) that allows for multiple levels of reducerMap. Since this function would have access to the entire tree of reducers, it could pass each reducer the global state regardless of high deep it is in the tree. For example, you would call it as such:

combineReducerTree({
  foo : fooReducerFn,
  bar : {
    baz: bazReducerFn,
    buzz: buzzReducerFn
  }
})

Using this would actually be a more declarative way of expressing multiple levels of reducers than just manually applying combineReducers() multiple times. Conveniently, combineReducerTree() would just be a logical superset of combineReducers() so if you so chose you could just replace the function.

If you like this idea I can expand this pull request to implement such a function.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson May 25, 2016

Contributor

I feel like part of the issue is that combineReducers is a default utility included in Redux, the standard approach suggested by the docs is splitting reducer logic by domain, and combineReducers is the standard way to handle that. As a result, everyone seems to assume that combineReducers is the only way to write a reducer, and then gets upset when they go past its limited use case.

A large part of me says that issues like the nested-tree problem you mention show that that structuring reducer logic is a very app-specific issue, and that modifying combineReducers is not a good answer. Admittedly, I'm not entirely sure I have a better answer right this minute.

Can this be dealt with more at the documentation level? I keep yammering about wanting to put together a "Structuring Reducers" recipe page, and I just haven't had time to do so. The related FAQ question is useful, but I really want to have a much larger page demonstrating actual various approaches to building more involved reducers.

I also feel like the reduce-reducers utility is pretty applicable here. Use combineReducers for your "simpler" per-domain update logic, use reduce-reducers for your more "cross-cutting" concerns.

Contributor

markerikson commented May 25, 2016

I feel like part of the issue is that combineReducers is a default utility included in Redux, the standard approach suggested by the docs is splitting reducer logic by domain, and combineReducers is the standard way to handle that. As a result, everyone seems to assume that combineReducers is the only way to write a reducer, and then gets upset when they go past its limited use case.

A large part of me says that issues like the nested-tree problem you mention show that that structuring reducer logic is a very app-specific issue, and that modifying combineReducers is not a good answer. Admittedly, I'm not entirely sure I have a better answer right this minute.

Can this be dealt with more at the documentation level? I keep yammering about wanting to put together a "Structuring Reducers" recipe page, and I just haven't had time to do so. The related FAQ question is useful, but I really want to have a much larger page demonstrating actual various approaches to building more involved reducers.

I also feel like the reduce-reducers utility is pretty applicable here. Use combineReducers for your "simpler" per-domain update logic, use reduce-reducers for your more "cross-cutting" concerns.

@eremzeit

This comment has been minimized.

Show comment
Hide comment
@eremzeit

eremzeit May 25, 2016

@markerikson Agreed that combineReducers is just an included utility and if I want something else I could write my own. However, if I were to write my own to cover my use case, 95% of it would be the same approach of combineReducers(). It'd still force state changes by reducers to be under a path and it'd still do the same sort of sanity checks on state shape. It'd just go a bit further and support reading a global state (lesser evil than duplicating references to global resources on each path) and combining multiple levels of reducers in a single call. So it's a question of whether you'd rather have it be spun it off into a separate reducer utility library or to just have combineReducers extended in a backwards compatible fashion to support the use case.

eremzeit commented May 25, 2016

@markerikson Agreed that combineReducers is just an included utility and if I want something else I could write my own. However, if I were to write my own to cover my use case, 95% of it would be the same approach of combineReducers(). It'd still force state changes by reducers to be under a path and it'd still do the same sort of sanity checks on state shape. It'd just go a bit further and support reading a global state (lesser evil than duplicating references to global resources on each path) and combining multiple levels of reducers in a single call. So it's a question of whether you'd rather have it be spun it off into a separate reducer utility library or to just have combineReducers extended in a backwards compatible fashion to support the use case.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson May 25, 2016

Contributor

So as far as this specific PR goes: my personal expectation for such an argument would actually be that the last argument would always be the global state, not one level up, and that it would be the previous state.

Contributor

markerikson commented May 25, 2016

So as far as this specific PR goes: my personal expectation for such an argument would actually be that the last argument would always be the global state, not one level up, and that it would be the previous state.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 25, 2016

Collaborator

I’m feeling that if we do something like this, we need to update all examples and documentation to use selectors (the regular ones, not Reselect) so it’s clear how to read safely from a global value without introducing crazy dependencies between reducers.

Collaborator

gaearon commented May 25, 2016

I’m feeling that if we do something like this, we need to update all examples and documentation to use selectors (the regular ones, not Reselect) so it’s clear how to read safely from a global value without introducing crazy dependencies between reducers.

@eremzeit

This comment has been minimized.

Show comment
Hide comment
@eremzeit

eremzeit May 29, 2016

In the latest commit I implemented the function I was imagining. It should be backwards compatible.

@gaearon By "regular selectors" do you just mean selectors implemented manually without an external library? Hmm, taking the idea of selectors one step further, I could also modify combineReducers to be callable like this:

combineReducers({
    myCache: myCacheReducerFn,
    foo: fooReducerFn,
    bar: barReducerFn,
}, (topLevelState) => {    //a selector function shapes the state
  return topLevelState.myCache
}

In the latest commit I implemented the function I was imagining. It should be backwards compatible.

@gaearon By "regular selectors" do you just mean selectors implemented manually without an external library? Hmm, taking the idea of selectors one step further, I could also modify combineReducers to be callable like this:

combineReducers({
    myCache: myCacheReducerFn,
    foo: fooReducerFn,
    bar: barReducerFn,
}, (topLevelState) => {    //a selector function shapes the state
  return topLevelState.myCache
}
@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson May 29, 2016

Contributor

Hmm. I could see some use for some of these changes. So if I'm following this correctly, at the moment you're proposing / have implemented three major changes:

  • combineReducers can now take a nested object tree of reducers
  • The top level state is now passed to every nested reducer
  • You're also suggesting that we could support taking a selector function as a second argument to combineReducers

I could sorta maybe get behind those changes. Still a couple concerns.

First, taking in a nested tree solves some use cases, but as @gaearon said, doesn't help with the specific use case of actually having called combineReducers multiple times. In other words, if I do this:

const rootReducer = combineReducers({
    a : reducerA,
    b : combineReducers({
        b1 : reducerB1,
        b2 : reducerB2
    })
});

Then reducer B1 wouldn't actually get the complete state, would it? Does that mean that cases like dynamically adding sub-reducers wouldn't ever be able to get the complete state? The idea of having a selector function as an argument seems sorta-kinda useful, but same potential concern.

Second, I still think the additional "complete state" value should be the previous state. I'm not sure I follow the reasoning for wanting it to be the "next state", or even how that's useful. Ah... looks like that approach was removed in the later commits, though, so I may be complaining about something that's now out of date.

For reference, here's a few reducer libs that demonstrate some related prior art:

Contributor

markerikson commented May 29, 2016

Hmm. I could see some use for some of these changes. So if I'm following this correctly, at the moment you're proposing / have implemented three major changes:

  • combineReducers can now take a nested object tree of reducers
  • The top level state is now passed to every nested reducer
  • You're also suggesting that we could support taking a selector function as a second argument to combineReducers

I could sorta maybe get behind those changes. Still a couple concerns.

First, taking in a nested tree solves some use cases, but as @gaearon said, doesn't help with the specific use case of actually having called combineReducers multiple times. In other words, if I do this:

const rootReducer = combineReducers({
    a : reducerA,
    b : combineReducers({
        b1 : reducerB1,
        b2 : reducerB2
    })
});

Then reducer B1 wouldn't actually get the complete state, would it? Does that mean that cases like dynamically adding sub-reducers wouldn't ever be able to get the complete state? The idea of having a selector function as an argument seems sorta-kinda useful, but same potential concern.

Second, I still think the additional "complete state" value should be the previous state. I'm not sure I follow the reasoning for wanting it to be the "next state", or even how that's useful. Ah... looks like that approach was removed in the later commits, though, so I may be complaining about something that's now out of date.

For reference, here's a few reducer libs that demonstrate some related prior art:

@eremzeit

This comment has been minimized.

Show comment
Hide comment
@eremzeit

eremzeit May 30, 2016

Wow thanks for the quick turnaround time. 👍

You mention the use case of calling combineReducers() multiple times like this:

const rootReducer = combineReducers({
    a : reducerA,
    b : combineReducers({
        b1 : reducerB1,
        b2 : reducerB2
    })
});

It's true that, as I had implemented so for in the PR, if you had setup the reducer like this then reducerB1 wouldn't have had access to the top level state as the third argument, but rather it would have received state.b. Good point. I just pushed a commit that addresses this. Now the function that combineReducers() returns takes a third argument. If the third argument is present then it passes that to it's sub-reducers as the top level state instead of the state that it was given. This allows combineReducers() to be chainable as in the code above but still correctly pass the top level state (or the "selected" state if we want to allow that) to the sub-reducers.

However, I see this as generally moot because with combineReducers() being able to support a multi-level reducer tree you'd implement the code above instead as the following code, which I view as more declarative.

const rootReducer = combineReducers({
    a : reducerA,
    b : {
        b1 : reducerB1,
        b2 : reducerB2
    }
});

In this case, if you wanted the logic below b to be defined in a different module, then you'd just have that other module return an object of reducers instead of a function.

You're also correct that in this version I pass the previous top-level state to the sub-reducers (rather than the next). You've convinced me that this makes more sense given that we're passing the reducers as an object and object keys aren't ordered. Anyways, it seems dubious to have one reducer depending on changes made by another reducer in the same tick unless you have a grammar for defining dependencies as in https://github.com/KodersLab/topologically-combine-reducers). Otherwise, if someone needed to do that they could just wrap their reducer function and handle that themselves.

Wow thanks for the quick turnaround time. 👍

You mention the use case of calling combineReducers() multiple times like this:

const rootReducer = combineReducers({
    a : reducerA,
    b : combineReducers({
        b1 : reducerB1,
        b2 : reducerB2
    })
});

It's true that, as I had implemented so for in the PR, if you had setup the reducer like this then reducerB1 wouldn't have had access to the top level state as the third argument, but rather it would have received state.b. Good point. I just pushed a commit that addresses this. Now the function that combineReducers() returns takes a third argument. If the third argument is present then it passes that to it's sub-reducers as the top level state instead of the state that it was given. This allows combineReducers() to be chainable as in the code above but still correctly pass the top level state (or the "selected" state if we want to allow that) to the sub-reducers.

However, I see this as generally moot because with combineReducers() being able to support a multi-level reducer tree you'd implement the code above instead as the following code, which I view as more declarative.

const rootReducer = combineReducers({
    a : reducerA,
    b : {
        b1 : reducerB1,
        b2 : reducerB2
    }
});

In this case, if you wanted the logic below b to be defined in a different module, then you'd just have that other module return an object of reducers instead of a function.

You're also correct that in this version I pass the previous top-level state to the sub-reducers (rather than the next). You've convinced me that this makes more sense given that we're passing the reducers as an object and object keys aren't ordered. Anyways, it seems dubious to have one reducer depending on changes made by another reducer in the same tick unless you have a grammar for defining dependencies as in https://github.com/KodersLab/topologically-combine-reducers). Otherwise, if someone needed to do that they could just wrap their reducer function and handle that themselves.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson May 30, 2016

Contributor

My couple vague-ish concerns at this point are:

  1. Not wanting to break existing multi-tiered uses of combineReducers
  2. Not breaking anything that has to do with replacing reducers, or dynamically adding/removing them

I don't have any specific concrete examples or test cases I can point to at the moment, just mental unease.

Thoughts?

Contributor

markerikson commented May 30, 2016

My couple vague-ish concerns at this point are:

  1. Not wanting to break existing multi-tiered uses of combineReducers
  2. Not breaking anything that has to do with replacing reducers, or dynamically adding/removing them

I don't have any specific concrete examples or test cases I can point to at the moment, just mental unease.

Thoughts?

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 1, 2016

Contributor

One more very relevant piece of prior art: https://github.com/ryo33/combine-section-reducers looks like it's doing just about the same thing as this PR.

Contributor

markerikson commented Jun 1, 2016

One more very relevant piece of prior art: https://github.com/ryo33/combine-section-reducers looks like it's doing just about the same thing as this PR.

@eremzeit

This comment has been minimized.

Show comment
Hide comment
@eremzeit

eremzeit Jun 1, 2016

There is no set of passed arguments that would have been valid for the previous version of combineReducers() that now would produce a different result when passed to the current version. This is because if you think of the new implementation as producing a reducer function from a tree of reducers of depth N, then the old implementation did exactly the same thing except only supported a depth of N=1 (ie. only supporting a mapping).

To support this claim, if I run the implementation of this PR against the specs of the master branch, only one test breaks, which is ignores all props which are not a function. This only breaks because to support a tree of reducers of arbitrary depth I need to loosen the restriction to instead be ignores all props which are not a function or object.

Or, looking at the code, if you just assume that the argument passed to combineReducers() is a mapping rather than a tree, then the code flows nearly identically to the old implementation.

If all of this is true, then the new implementation is backwards compatible and then both of your concerns are covered. All of the new behavior is essentially opt-in, whether that be: a) accessing the global state from a sub-reducer or b) passing a reducer tree of arbitrary depth to combineReducers.

The library you mention (https://github.com/ryo33/combine-section-reducers) is similar in that it allow you to pass the global state to a reducer, but is different in that it doesn't support a reducer tree of arbitrary depth. Also, it doesn't support the same sanity checks on state shape that combineReducers() does.

eremzeit commented Jun 1, 2016

There is no set of passed arguments that would have been valid for the previous version of combineReducers() that now would produce a different result when passed to the current version. This is because if you think of the new implementation as producing a reducer function from a tree of reducers of depth N, then the old implementation did exactly the same thing except only supported a depth of N=1 (ie. only supporting a mapping).

To support this claim, if I run the implementation of this PR against the specs of the master branch, only one test breaks, which is ignores all props which are not a function. This only breaks because to support a tree of reducers of arbitrary depth I need to loosen the restriction to instead be ignores all props which are not a function or object.

Or, looking at the code, if you just assume that the argument passed to combineReducers() is a mapping rather than a tree, then the code flows nearly identically to the old implementation.

If all of this is true, then the new implementation is backwards compatible and then both of your concerns are covered. All of the new behavior is essentially opt-in, whether that be: a) accessing the global state from a sub-reducer or b) passing a reducer tree of arbitrary depth to combineReducers.

The library you mention (https://github.com/ryo33/combine-section-reducers) is similar in that it allow you to pass the global state to a reducer, but is different in that it doesn't support a reducer tree of arbitrary depth. Also, it doesn't support the same sanity checks on state shape that combineReducers() does.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 1, 2016

Contributor

Okay, sounds good. Can we get some doc updates to match the code?

Contributor

markerikson commented Jun 1, 2016

Okay, sounds good. Can we get some doc updates to match the code?

@eremzeit

This comment has been minimized.

Show comment
Hide comment
@eremzeit

eremzeit Jun 9, 2016

FYI I haven't forgotten about this. Work has been crazy, but I'm hoping to get to this this coming week.

eremzeit commented Jun 9, 2016

FYI I haven't forgotten about this. Work has been crazy, but I'm hoping to get to this this coming week.

eremzeit added some commits Jun 15, 2016

Update combineReducers.md
Updating combineReducers.md to reflect how `combineReducers` was expanded to support recursive reducer trees.
@eremzeit

This comment has been minimized.

Show comment
Hide comment
@eremzeit

eremzeit Jun 15, 2016

I updated the docs section on combineReducers.

I pondered also updating the section on basic reducers, but I felt like the goal of the section was clearly outline the concept of reducing and that rather than distracting from that I should leave recursive reducer trees as an advanced feature left for the API docs.

I updated the docs section on combineReducers.

I pondered also updating the section on basic reducers, but I felt like the goal of the section was clearly outline the concept of reducing and that rather than distracting from that I should leave recursive reducer trees as an advanced feature left for the API docs.

@eremzeit

This comment has been minimized.

Show comment
Hide comment
@eremzeit

eremzeit Jun 15, 2016

I also was looking at #1792. There's nothing inherently incompatible between those proposed changes and what I've done here. I could wait until someone gets a PR merged for that and then I could merge those into this PR. Or alternatively I could just directly add that feature (which would look something like borkxs@7b3315e but ) into this PR. Either way.

I also was looking at #1792. There's nothing inherently incompatible between those proposed changes and what I've done here. I could wait until someone gets a PR merged for that and then I could merge those into this PR. Or alternatively I could just directly add that feature (which would look something like borkxs@7b3315e but ) into this PR. Either way.

@ryo33

This comment has been minimized.

Show comment
Hide comment
@ryo33

ryo33 Jun 20, 2016

@eremzeit
Let me tell you that combineSectionReducers supports nested state tree and sanity checks on state shape.

ryo33 commented Jun 20, 2016

@eremzeit
Let me tell you that combineSectionReducers supports nested state tree and sanity checks on state shape.

@eremzeit

This comment has been minimized.

Show comment
Hide comment
@eremzeit

eremzeit Jun 21, 2016

The library you mention (https://github.com/ryo33/combine-section-reducers) is similar in that it allow you to pass the global state to a reducer, but is different in that it doesn't support a reducer tree of arbitrary depth. Also, it doesn't support the same sanity checks on state shape that combineReducers() does.

Apologies for the confusion. The difference is that this PR implements combineReducers to be recursive such that it can be used on more than one level of reducer. Ie:

combineReducers({
  foo: {
    bar: {
      baz:  <reducer function>
    }
  }
})

Also, it's implemented in a backwards compatible way into the original combineReducers so the user doesn't have to decide when to use combineReducers or combineSectionReducers.

In regards to sanity checks, I just meant those that were already included with combineReducers are a bit more comprehensive than the ones in is-valid-redux-reducer.

The library you mention (https://github.com/ryo33/combine-section-reducers) is similar in that it allow you to pass the global state to a reducer, but is different in that it doesn't support a reducer tree of arbitrary depth. Also, it doesn't support the same sanity checks on state shape that combineReducers() does.

Apologies for the confusion. The difference is that this PR implements combineReducers to be recursive such that it can be used on more than one level of reducer. Ie:

combineReducers({
  foo: {
    bar: {
      baz:  <reducer function>
    }
  }
})

Also, it's implemented in a backwards compatible way into the original combineReducers so the user doesn't have to decide when to use combineReducers or combineSectionReducers.

In regards to sanity checks, I just meant those that were already included with combineReducers are a bit more comprehensive than the ones in is-valid-redux-reducer.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 21, 2016

Collaborator

I think we shouldn’t just go ahead and add this because it seems useful in some situations. If we add this, we maintain this forever. It’s a big cost if we later discover there are some problems with this API or if there are bugs to be fixed.

I would rather see this as a third-party library. If it’s truly useful, you should have no problem submitting pull requests to boilerplate projects, examples and open source apps that would benefit from it. If it gains enough popularity, we can consider replacing combineReducers() with it.

Collaborator

gaearon commented Jun 21, 2016

I think we shouldn’t just go ahead and add this because it seems useful in some situations. If we add this, we maintain this forever. It’s a big cost if we later discover there are some problems with this API or if there are bugs to be fixed.

I would rather see this as a third-party library. If it’s truly useful, you should have no problem submitting pull requests to boilerplate projects, examples and open source apps that would benefit from it. If it gains enough popularity, we can consider replacing combineReducers() with it.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 21, 2016

Contributor

I certainly agree that there's more than enough demand out there to consider such a change. The biggest question, as we've seen is what the semantics of the additional argument should look like, and that's where things start getting complicated.

I still feel like part of the issue is user education that combineReducers isn't the be-all-end-all of defining reducer logic, but that seems like sort of a lost cause at this point.

Contributor

markerikson commented Jun 21, 2016

I certainly agree that there's more than enough demand out there to consider such a change. The biggest question, as we've seen is what the semantics of the additional argument should look like, and that's where things start getting complicated.

I still feel like part of the issue is user education that combineReducers isn't the be-all-end-all of defining reducer logic, but that seems like sort of a lost cause at this point.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 22, 2016

Collaborator

(Note that I do sort of like the API. But I would like to see real world usage before it becomes something we ship to thousands of users.)

Collaborator

gaearon commented Jun 22, 2016

(Note that I do sort of like the API. But I would like to see real world usage before it becomes something we ship to thousands of users.)

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Jul 19, 2016

Member

I was actually just discussing this with a coworker this afternoon. In the new product we're building, he's ended up designing a state tree with a very overloaded key in order to cross a lot of boundaries. Unfortunately, combineReducers makes it easy to make these kinds of mistakes (he's a senior-level dev too). I worry other teams are going to start building big projects with tools that are better suited for smaller apps.

Why not fold-in and provide reduce-reducers as a first-class citizen? That would signal that there are multiple ways to build your state tree depending on your app scope and needs.

Member

timdorr commented Jul 19, 2016

I was actually just discussing this with a coworker this afternoon. In the new product we're building, he's ended up designing a state tree with a very overloaded key in order to cross a lot of boundaries. Unfortunately, combineReducers makes it easy to make these kinds of mistakes (he's a senior-level dev too). I worry other teams are going to start building big projects with tools that are better suited for smaller apps.

Why not fold-in and provide reduce-reducers as a first-class citizen? That would signal that there are multiple ways to build your state tree depending on your app scope and needs.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jul 19, 2016

Contributor

@timdorr : that's part of what I'm hoping to clarify with the "Structuring Reducers" doc I'm (intermittently) working on. See #1784 . I'd certainly be interested in any feedback on the intended content, the WIP so far, and actual use cases and approaches people are coming up with.

I'm assuming that you're referring to something like an "entities" slice, where it's less "specific subset of domain data" and more "ALL the domain data", hence involved in all kinds of manipulations? What "mistakes" are you referring to?

Contributor

markerikson commented Jul 19, 2016

@timdorr : that's part of what I'm hoping to clarify with the "Structuring Reducers" doc I'm (intermittently) working on. See #1784 . I'd certainly be interested in any feedback on the intended content, the WIP so far, and actual use cases and approaches people are coming up with.

I'm assuming that you're referring to something like an "entities" slice, where it's less "specific subset of domain data" and more "ALL the domain data", hence involved in all kinds of manipulations? What "mistakes" are you referring to?

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Jul 19, 2016

Member

Ah, that's a great start! I still think it's a combination of docs and signals via the APIs we provide that will get folks to unlearn bad habits.

But I'm about to have a baby in about 36 hours, so my time is going to be really limited. But I'll give it a look-see when I'm back to reality!

I'm assuming that you're referring to something like an "entities" slice, where it's less "specific subset of domain data" and more "ALL the domain data", hence involved in all kinds of manipulations? What "mistakes" are you referring to?

Yeah, we have a search widget that was containing both the search criteria and the results data under one key. It was really starting to get messy and my coworker was becoming painfully aware of how he was cargo culting combineReducers. Luckily, we're still early enough along that it's not going to be a spinal transplant to fix.

Member

timdorr commented Jul 19, 2016

Ah, that's a great start! I still think it's a combination of docs and signals via the APIs we provide that will get folks to unlearn bad habits.

But I'm about to have a baby in about 36 hours, so my time is going to be really limited. But I'll give it a look-see when I'm back to reality!

I'm assuming that you're referring to something like an "entities" slice, where it's less "specific subset of domain data" and more "ALL the domain data", hence involved in all kinds of manipulations? What "mistakes" are you referring to?

Yeah, we have a search widget that was containing both the search criteria and the results data under one key. It was really starting to get messy and my coworker was becoming painfully aware of how he was cargo culting combineReducers. Luckily, we're still early enough along that it's not going to be a spinal transplant to fix.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jul 19, 2016

Contributor

@timdorr: Hey, slightly early congratulations, then!

I wrote much of the FAQ page during a 3-week business trip back in March. I'm on a 2-week trip right now, so I'm hoping that if nothing else I can sit down this weekend and crank out a pretty good chunk of writing.

Contributor

markerikson commented Jul 19, 2016

@timdorr: Hey, slightly early congratulations, then!

I wrote much of the FAQ page during a 3-week business trip back in March. I'm on a 2-week trip right now, so I'm hoping that if nothing else I can sit down this weekend and crank out a pretty good chunk of writing.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Nov 2, 2016

Contributor

@gaearon , @timdorr , @jimbolla , @eremzeit :

This PR has been quiet for the last few months. In the meantime, we've had a couple more PRs filed requesting changes to combineReducers. I think we need to figure out what we're doing pretty soon, as part of planning for that semi-hypothetical 4.0 milestone.

It seems like we have four major options:

  • Make combineReducers a whole lot more flexible so that people can customize it as needed
  • Pick one or two specific suggested changes and implement them
  • Break combineReducers into pieces that could be assembled together and reused
  • Just draw a line and say "nope, that's it, it's done, and if you want to do something else, go write your own".

Thoughts?

Contributor

markerikson commented Nov 2, 2016

@gaearon , @timdorr , @jimbolla , @eremzeit :

This PR has been quiet for the last few months. In the meantime, we've had a couple more PRs filed requesting changes to combineReducers. I think we need to figure out what we're doing pretty soon, as part of planning for that semi-hypothetical 4.0 milestone.

It seems like we have four major options:

  • Make combineReducers a whole lot more flexible so that people can customize it as needed
  • Pick one or two specific suggested changes and implement them
  • Break combineReducers into pieces that could be assembled together and reused
  • Just draw a line and say "nope, that's it, it's done, and if you want to do something else, go write your own".

Thoughts?

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Nov 2, 2016

Member

I'm a fan of composition vs. expanding an API. If we could break up combineReducers and let users put it back together however they want (along with maintaining the existing API), that would be the most scalable option.

Member

timdorr commented Nov 2, 2016

I'm a fan of composition vs. expanding an API. If we could break up combineReducers and let users put it back together however they want (along with maintaining the existing API), that would be the most scalable option.

@jimbolla

This comment has been minimized.

Show comment
Hide comment
@jimbolla

jimbolla Nov 2, 2016

Contributor

I think there's potential to model combineReducers off reselect's createSelector or redux thunk middleware where there's a factory function that creates the default implementation (createSelector/thunk) but advanced use cases can be supported if the user calls the factory (createSelectorCreator/thunk.withExtraArgument) with custom args. So we could have, for example const customCombine = combineReducers.customize({ options }); or something like that. This way, combineReducers itself continues to behave as-is, but some customization/reuse is also achievable.

Contributor

jimbolla commented Nov 2, 2016

I think there's potential to model combineReducers off reselect's createSelector or redux thunk middleware where there's a factory function that creates the default implementation (createSelector/thunk) but advanced use cases can be supported if the user calls the factory (createSelectorCreator/thunk.withExtraArgument) with custom args. So we could have, for example const customCombine = combineReducers.customize({ options }); or something like that. This way, combineReducers itself continues to behave as-is, but some customization/reuse is also achievable.

@lukescott

This comment has been minimized.

Show comment
Hide comment
@lukescott

lukescott Oct 26, 2017

I'm not really a fan of passing global state because it breaks encapsulation. IMO, combineReducers defines a sub-state (or component) of the application. That sub-state should not know anything about the parent.

Using a selector to pass a third argument, whether that be from the top-level state or not, that's a bit more interesting. However, I'm hesitant about that because it breaks the pureness of a (state, action) reducer. Whether or not that's actually important or not, I'm not sure. I've actually played with this idea in a project I'm working on, and it always came back to that for me.

I actually abandoned the use of combineReducers in my project in favor of a simple combine function:

// TypeScript:
export function combine<T>(a: T, b: Partial<T>): T {
	const keys = Object.keys(b) as (keyof T)[]
	if (keys.some(k => a[k] !== b[k])) {
		return Object.assign({}, a, b)
	}
	return a
}

Which I use like this:

const reducer = (state, action) => combine(state, {
  a: reducerA(state.a, action),
  b: reducerB(state.b, action),
})

// vs

const reducer combineReducers({
  a: reducerA,
  b: reducerB,
})

It's slightly more code, but overall it was easier to follow, and allows someone to more easily operate on the top-level state if needed.

I'm not really a fan of passing global state because it breaks encapsulation. IMO, combineReducers defines a sub-state (or component) of the application. That sub-state should not know anything about the parent.

Using a selector to pass a third argument, whether that be from the top-level state or not, that's a bit more interesting. However, I'm hesitant about that because it breaks the pureness of a (state, action) reducer. Whether or not that's actually important or not, I'm not sure. I've actually played with this idea in a project I'm working on, and it always came back to that for me.

I actually abandoned the use of combineReducers in my project in favor of a simple combine function:

// TypeScript:
export function combine<T>(a: T, b: Partial<T>): T {
	const keys = Object.keys(b) as (keyof T)[]
	if (keys.some(k => a[k] !== b[k])) {
		return Object.assign({}, a, b)
	}
	return a
}

Which I use like this:

const reducer = (state, action) => combine(state, {
  a: reducerA(state.a, action),
  b: reducerB(state.b, action),
})

// vs

const reducer combineReducers({
  a: reducerA,
  b: reducerB,
})

It's slightly more code, but overall it was easier to follow, and allows someone to more easily operate on the top-level state if needed.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Oct 26, 2017

Contributor

@lukescott : a pure function that takes (state, action, someOtherValue) is still a valid "reducer function", it just doesn't fit the exact specific signature that the root reducer has or that combineReducers expects for its slice reducers.

Yes, the intended usage of combineReducers is that each slice is independent, but in real world apps that approach may not always work (hence the numerous requests for alternate approaches). You should always feel free to organize your reducer logic differently if it works better for your situation.

Contributor

markerikson commented Oct 26, 2017

@lukescott : a pure function that takes (state, action, someOtherValue) is still a valid "reducer function", it just doesn't fit the exact specific signature that the root reducer has or that combineReducers expects for its slice reducers.

Yes, the intended usage of combineReducers is that each slice is independent, but in real world apps that approach may not always work (hence the numerous requests for alternate approaches). You should always feel free to organize your reducer logic differently if it works better for your situation.

@timdorr timdorr removed this from the 4.0 milestone Nov 3, 2017

@phationmationion

This comment has been minimized.

Show comment
Hide comment
@phationmationion

phationmationion Nov 18, 2017

I think that possibility to set in projects only one combineReducers-like function, which takes all reducersTree - it's good idea. And more: absence of necessity to combine reducers in project - it's just great. If Redux can to combine all reducers instead of developers - shouldn't Redux to have that feature?

import combineReducers from './combineReducers'

export default function createRootReducer(reducersTreeNode) {
   if (typeof reducersTreeNode === 'function')
      return reducersTreeNode

   Object.keys(reducersTreeNode).forEach(key => {
      reducersTreeNode[key] = createRootReducer(reducersTreeNode[key])
   })
   return combineReducers(reducersTreeNode)
}

Example of usage here (shopping-cart example): phationmationion@eb899d6

And just idea: remove combineReducers form exports of Redux and inject createRootReducer here

// ...code...
let currentReducer = createRootReducer(reducer)

phationmationion commented Nov 18, 2017

I think that possibility to set in projects only one combineReducers-like function, which takes all reducersTree - it's good idea. And more: absence of necessity to combine reducers in project - it's just great. If Redux can to combine all reducers instead of developers - shouldn't Redux to have that feature?

import combineReducers from './combineReducers'

export default function createRootReducer(reducersTreeNode) {
   if (typeof reducersTreeNode === 'function')
      return reducersTreeNode

   Object.keys(reducersTreeNode).forEach(key => {
      reducersTreeNode[key] = createRootReducer(reducersTreeNode[key])
   })
   return combineReducers(reducersTreeNode)
}

Example of usage here (shopping-cart example): phationmationion@eb899d6

And just idea: remove combineReducers form exports of Redux and inject createRootReducer here

// ...code...
let currentReducer = createRootReducer(reducer)
@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Nov 18, 2017

Contributor

@phationmationion : for what it's worth, there are several existing third-party reducer utilities that do that kind of thing already, and I linked some of them upthread. Also see the Reducers section of my Redux addons catalog.

There's also no need to "inject a reducer creator into createStore. You are responsible for creating your root reducer function, and you can do that however you want. You can use combineReducers, combineSectionReducers, reduceReducers, or any other logic you want to create that root reducer. All that is done outside of the store.

Contributor

markerikson commented Nov 18, 2017

@phationmationion : for what it's worth, there are several existing third-party reducer utilities that do that kind of thing already, and I linked some of them upthread. Also see the Reducers section of my Redux addons catalog.

There's also no need to "inject a reducer creator into createStore. You are responsible for creating your root reducer function, and you can do that however you want. You can use combineReducers, combineSectionReducers, reduceReducers, or any other logic you want to create that root reducer. All that is done outside of the store.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Nov 18, 2017

Contributor

Given that this is no longer part of the 4.0 milestone, that the original author of the PR has disappeared, and that there doesn't seem to be anyone who has expressed an interest in pushing it forward, I'm going to close this PR.

If someone actually does want to tackle improving combineReducers, either by expanding on this PR or coming at it from a different direction like making it out of composable pieces, please let me know and we can discuss how to approach it.

Contributor

markerikson commented Nov 18, 2017

Given that this is no longer part of the 4.0 milestone, that the original author of the PR has disappeared, and that there doesn't seem to be anyone who has expressed an interest in pushing it forward, I'm going to close this PR.

If someone actually does want to tackle improving combineReducers, either by expanding on this PR or coming at it from a different direction like making it out of composable pieces, please let me know and we can discuss how to approach it.

@phationmationion

This comment has been minimized.

Show comment
Hide comment
@phationmationion

phationmationion Nov 20, 2017

@markerikson sorry, I've wrote a big message about reducersTree at start, but then understood that reducersTree can't to provide rootReducer-wrappers (for example, "full-state-reset-wrapper") and it's very big missing. Now I can see: rootReducer in projects is really better than just reducersMap. Thanks for feedback!

phationmationion commented Nov 20, 2017

@markerikson sorry, I've wrote a big message about reducersTree at start, but then understood that reducersTree can't to provide rootReducer-wrappers (for example, "full-state-reset-wrapper") and it's very big missing. Now I can see: rootReducer in projects is really better than just reducersMap. Thanks for feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment