Throw error on undefined value from store function

[taylorhakes]

Updated composeStores to throw if undefined is returned. #191

[ooflorent]

Why didn't you use an arrow function here?

[gaearon]

This is "probing" a reducer but I actually want the check to run every time a reducer is called. So it needs to be inside the function actually calling it.

[gaearon]

Also, because it is a hard constraint, it should be enforced in production too. No need for the if statement. Otherwise pros and dev bugs would differ.

[taylorhakes]

I changed it because I was considering unexpected issues in production. For instance, if the default doesn't return undefined, but one of their case statements. This could cause their whole state not to update. It is debatable whether an undefined state is worse than causing a full update not to work.

I think it generally shouldn't be an issue in dev. I think that someone might run into the error at the wrong time in production, unfortunately.

[taylorhakes]

I made that change to your suggestion. I could be wrong and people will prefer a hard error, even in production.

[ooflorent]

I'm not very fond of undefined since it may be overridden.

var undefined = null
console.log(undefined) // null

void 0 is less readable but works as expected.

[taylorhakes]

I thought about doing that, but I wasn't sure the stance of this repo. I would also need to change it in composeStores.js. What are people's thoughts?

[gaearon]

Just return is enough :-).

[gaearon]

If something warns in development it must no-op in production.
If something throws in development it must throw in production.

You're not going to "save" the app by returning undefined in production. It's already unexpected what will happen (because it never happened in dev apparently).

It's important to keep crashes consistent. Otherwise you won't be able to reproduce a production crash report in development. The different stack trace will mislead you and you'll have a hard time figuring out why you never have crashes with that stack trace in development.

[gaearon]

It looks like a hack. Can you please check typeof instead?

[taylorhakes]

This is a pretty common pattern. I will make the change. How should I fix the other UNDEF use in the test?

[gaearon]

You can replace return UNDEF with just return, and use typeof x === 'undefined' as a check.
Or are you referring to something else?

[taylorhakes]

Unfortunately, there is an ESLint rule that forbids return; when there is another type returned elsewhere. I think we can just use undefined though. We don't have to worry about users overwriting undefined in tests.

[gaearon]

I think you can also invert the condition.
Instead of returning undefined in some cases, return something in other cases.

function bad() {
  if (action.type !== 'something') {
    return something;
  }
}

[taylorhakes]

I thought about that. I was trying to make the test as obvious as possible. I will change it to above

[gaearon]

Here's a potential crazy issue you might encounter if undefined was tolerated in production: loss of user data. Instead of crashing, data is overridden, and the app saves the bad state.

Crashing isn't always bad :-)

[taylorhakes]

Definitely, I hinted above that there is a trade off. If the app crashes in prod, losing state might be better than blocking the action. Depends on the app.

I just trying to note that by fixing one issue, we are introducing another. Probably will be less an issue than the original. Time will tell.

I am fine making it a hard error. I understand your reasoning.

[gaearon]

@taylorhakes Once you get to that, would you please make another PR on top of #195? It contains the renaming of this function..

[taylorhakes]

I will need to close this pull request to make a pull request against the alpha branch.