Change composeStores to throw if undefined is returned

[gaearon]

Right now it's easy to forget a default case and have your reducer return nothing in some circumstances. There have been some WIP attempts to address it: #174. Here's the issue that spawned that PR: #173.

But let's take a moment to think about what composeStores really is. It's a shortcut right? It's a bit opinionated already: it assumes plain objects as the initial state. See #153.

Can we make it a bit more opinionated for convenience? We could make it return the previous state if the reducer returns undefined.

Edit: see this instead.

Pros:

• Solves issues like Reducers (ex-'stores') must have a default return value for state to be initialized properly? #173 nicely
• Slightly less boilerplate in the “vanilla Redux” code

Cons:

• Dilutes the contract of reducers by introducing some magic

Thoughts?

[gaearon]

To be honest I can't imagine undefined being a valid state. If you want “nothing” just return null.
Therefore I think we should do something with undefined:

• Either interpret it as “keep the previous state” as suggested above
• Or throw.

[gaearon]

Here's something wrong with this change. People will start with composeStores but might wrap their reducers into community-provided helpers (e.g. DevTools, pagination utils, etc) later. If all of these community-provided have to special-case undefined, it's going to be a pain. It's also easy to forget about this when writing a utility, and to interpret undefined as a value. Everyone's going to have to write special branches to handle this case.

Therefore I propose, instead of interpreting undefined as a special value, narrow down the composeStores-compatible reducer contract. That is, throw when undefined is return.

It's a breaking change, but one that is easy to fix with switch statements, and even easier with custom reducer factories (might require a single line change). I think it's worth it to keep the contract interoperable, but prevent mistakes like #173.

[vslinko]

I have a stores with undefined as a valid value. For example, tokenStore can store undefined or string.
I don't want to store null or any other object instead of undefined. Problems with missed default in switch can be easily fixed by any linter.

[johanneslumpe]

Do you really check for undefined or just !token ? Also, instead or just returning a single value you could wrap it in an object and keep your undefined value.

I would welcome this change. It is a bit more opinionated, but it will solve some common mistakes.

Sent from my iPhone

On 29 Jun 2015, at 19:29, Vyacheslav Slinko notifications@github.com wrote:

I'm have a stores with undefined as valid value. For example tokenStore stores undefined or string.
I don't want to store null or any other object.

—
Reply to this email directly or view it on GitHub.

[vslinko]

I don't want to wrap every single value with an object. I want to write const {token} = getState() instead of const {tokenStore: {token}} = getState() or const {token: {hash}} = getState().

Yes, I'm just checking for positive value !token, but it doesn't matter. undefined is more native than false or even null.
Furthermore, null is forbidden in my project.

[burmisov]

I welcome this change (in the latter edition), no undefined states on my side. If it threw immediately, it would be very clear what's wrong with your reducer code and in what place exactly. It would have saved me a lot of time debugging :)
Though it will cause some unexpected behavior when the store becomes undefined at production run time by mistake, but that can be mitigated developer-side in dangerous cases.
No undefined state allowed is a constraint, but I think it is a worthy trade-off.

@vslinko I think enforcing default case using a linter is good practice for sure, but the Redux semantics specifically require to return unchanged/initial state in this case, which is somewhat different from having some default handler (e.g. my typical default handler throws something like new Error("Unknown case .."):))

[rpominov]

Yes, I'm just checking for positive value !token, but it doesn't matter. undefined is more native than false or even null.
Furthermore, null is forbidden in my project.

I think null suits better in this case. undefined is absence of prove, and null is prove of absence.

[emmenko]

I'm not really sure about this. First of all, it's up to userland to correctly implement a switch statement. Like someone already suggested, just use a lint to check that. Also you don't have to use switch, for example you can simply do it like this.

But if we do want to check that on our (redux) side, then I would suggest to just log a warning like React does. We also have a proposal for that about mutable state #138

[gaearon]

@vslinko If it's really a valid use case for you and a matter of principle, you can always use your own composeReducers with your custom behavior. I still think the pros of this change, especially making it friendly to the beginners, outweigh the cons.

[gaearon]

But if we do want to check that on our (redux) side, then I would suggest to just log a warning like React does.

If we just log a warning we get the worst of both options in my opinion. People having the problem might fail toah notice it (because they'll be focused on the actual error which is likely to happen in the component), but people like @vslinko who actually want undefined to be a valid value will be annoyed because of the pointless warning when they know what they're doing.

Redux tries to be in opinionated and not include "shortcuts" if possible because community can build them just fine. I think that when we do include a shortcut like composeReducers we must prioritize beginners over experienced devs. Beginners are who these shortcuts are for. Experienced devs are likely to build their own anyway.

[gaearon]

I think null suits better in this case. undefined is absence of prove, and null is prove of absence.

This is also my stance.

Just forbidding null sounds like an unnecessary strict policy.
And in any case, as a power user, you can use your own helper anyway.

[vslinko]

Error is definitely better than warning.

[leoasis]

@gaearon would it make sense to make this shortcuts somehow removable from the total library footprint, so that experienced users won't need to have these unused shortcuts take bundle size?

Say, importing redux adds all beginner friendly stuff, but other users can somehow import the specific pieces without including the shortcuts code. This latter path should not be too verbose though.

[emmenko]

At the end, it's hard to make everyone happy :)

I'm also fine with throwing, was just trying to find a "soft" solution.

I guess we just have to try and see how it goes...

[gaearon]

We'll probably ask people to export stuff like this from redux/addons. Like React does it in React 0.14.

[gaearon]

Also, with React 0.14 we'll probably be able to kill redux/react and redux/react-native and be able to have a single component export. (ping @zpao to confirm this?)

Then we can put all “non-core” stuff into /addons:

import { createRedux } from 'redux';
import { Connector, Provider, composeReducers  } from 'redux/addons';

// can also import individually:
import Connector from 'redux/addons/Connector';

[emmenko]

👍

[leoasis]

awesome 👍

[taylorhakes]

Is anyone working on this? I can work on this change

[gaearon]

This is done now via #197.