Skip to content
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

Unnecessary "Unexpected key ..." warning when merging combined reducers #2427

Closed
mpeyper opened this issue May 28, 2017 · 4 comments
Closed

Comments

@mpeyper
Copy link
Contributor

mpeyper commented May 28, 2017

Do you want to request a feature or report a bug?

A bug... maybe. It seems like it's intended behaviour but I don't know who benefits from the warning.

It is similar to #1636, but not caused by replaceReducers, but by wanting to merge the state from seperate reducers, which have been combined with combineReducers.

What is the current behavior?

When combineReducers receives keys it doesn't know about it produces a warning stating

Unexpected key "additional" found in previous state received by the reducer. Expected to find one of the known reducer keys instead: "known". Unexpected keys will be ignored.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar.

Here is a codepen with a simplified example of what I'm doing.

What is the expected behavior?

I would not expect to see this warning, as I believe that merging state from multiple reducers is a valid use case for Redux. Even one of the examples in the Redux docs would have the same issue if the crossSliceReducer added a new key rather than overriding an existing one.

I don't really understand who is benefitting from this warning. If it was warning about a missing key (after it has previously been there and there was a slice reducer expecting it) I would understand that that is something that is unusual and probably a bug, but I don't see much harm in having additional nodes that the reducer is just going to ignore anyway.

Which versions of Redux, and which browser and OS are affected by this issue? Did this work in previous versions of Redux?

I am unsure which version of Redux introduced the warning and why, but it is still present in 3.6.0

@markerikson
Copy link
Contributor

The point of using combineReducers is that you are delegating the work of updating each slice of state to a specific slice reducer. As part of that, combineReducers is deliberately opinionated, and tries to point out potential errors. In this case, it's pointing out that at least one slice of state doesn't seem to actually have a slice reducer assigned to it, and therefore probably won't ever get updated.

I'm going to say this is working as intended. Dynamic use of combineReducers with a different number of slices is a relatively small use case, and most of the time even that is only adding new slice reducers, not using them. So, a key mismatch, intentional or unintentional, is going to be rare either way, and if it does happen it's more likely to be "wrong". In addition, the warnings are only actually logged in development.

It's also easy enough to create a copy-paste version of combineReducers that drops out the unexpected shape warnings, and use that instead, or use one of the dozens of other existing reducer-building utilities out there.

@mpeyper
Copy link
Contributor Author

mpeyper commented May 28, 2017

The point of using combineReducers is that you are delegating the work of updating each slice of state to a specific slice reducer. As part of that, combineReducers is deliberately opinionated...

I agree that it should be opinionated about what it does, but I guess I don't agree that it should so opinionated about what people do with it. After all, one of the huge advantages of Redux is that you can use function composition in all kinds of ways to create a reducer that produces the perfect state for your app.

In this case, it's pointing out that at least one slice of state doesn't seem to actually have a slice reducer assigned to it, and therefore probably won't ever get updated.

The problem with this statement is that there is only a small number of cases where it is true (and you've just said that it's not worth catering for a small number of cases), assuming all the keys for the combineReducers reducer(s) are present:

  1. You're using combineReducers from top to bottom and you've given an initial state that doesn't match - you may have a unwanted key, but you've still provided all the keys it was expecting.
  2. You're using combineReducers from top to bottom and not providing an initial state - this won't happen, because combineReducers wont let it happen.
  3. You're mixing combineReducers with a custom reducer builder or one of the dozens of other existing reducer-building utilities out there - it is more likely that the additional key is coming from the other utility, but it will be ignored by combineReducers anyway.
  4. You have built your reducer without combineReducers at all - obviously not at issue for you.

(If there are more scenarios you can identify where having an additional key is more likely to be a mistake then not, then please reply and let me know).

As I previously mentioned, the Redux docs even have a whole page dedicated to moving beyond combineReducers that includes mixing it with reduceReducers, which can easily add additional nodes. A simple example I can think of is having 2 collections that are managed by seperate reducers and combined together, and a third reducer managing cross cutting statistics on the two collections that is reduced together with the combined reducer.

In addition, the warnings are only actually logged in development.

I understand this but I'd rather not have a unnecessary warning produced at all (how many times have you spent tracking down an issue that isn't really an issue?).

It's also easy enough to create a copy-paste version of combineReducers that drops out the unexpected shape warnings, and use that instead, or use one of the dozens of other existing reducer-building utilities out there.

Unfortunately for me, I'm writing a library that must accept any reducer passed to it and they will often be created using combineReducers. The library is part of a much larger code-splitting effort on a very large app. The reducers that need to be merged are written by multiple teams, in multiple geographical locations, in different repos, so insisting they not use combineReducers, especially for those new to React and Redux, will be difficult to enforce, and honestly I don't want to limit them. As long as combineReducers remains in the Redux code base and be as well documented as the "correct" way to combine reducers, developers will use it as their preference (and so they should).

I also imagine that many of the utilities in that list are using the built-in combineReducers as part of what they do. The ability to mix and match those utilities with each other, as well as combineReducers should be a strength of Redux, not an annoyance.

Given how definite your answer was (closed within 2 hours of opening without any other discussion or input), I'm not actually expecting you to reconsider now, I just hope that some of my points are taken into account when looking at a solution for #1636.

@esamattis
Copy link

esamattis commented Jun 14, 2017

I've found the behaviour of combineReducers oddly opinionated too since Redux itself is not very opionated but this is.

I've often use a helper like this

function composeReducers(...reducers) {
    return (state = null, action) => {
        return reducers
            .filter(r => typeof r === "function")
            .reduce((state, subReducer) => subReducer(state, action), state);
    };
}

which can used to put multiple reducers to same part of the state.

const store = createStore(composeReducers(
  myReducer,
  myAnotherReducer,
  combineReducers(...) // does not work
));

Unfortunately this does not work with combineReducers if one of my* reducers creates a new top level new key. :/

@fangpenlin
Copy link

I am new to Redux, just followed the official document

http://redux.js.org/docs/recipes/reducers/BeyondCombineReducers.html

and ran into the same issue.

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

No branches or pull requests

4 participants