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

Fix unknown action type handling #1307

Closed
wants to merge 1 commit into from

Conversation

msafi
Copy link

@msafi msafi commented Jan 28, 2016

As discussed in #1054, and if I understand correctly, the private action handling test is not really possible if the user has access ActionTypes.INIT.

So, I changed the way combineReducers inits reducers by making it do a random probe and undefined state checks simultaneously.

I modified the tests to make sure combineReducers implementation throws if the given reducer could potentially return undefined either for an undefined state or an unknown action type.

@msafi msafi changed the title Fix unknown action type handling. Fix unknown action type handling Jan 28, 2016
@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2016

Hey, sorry for not getting back to this sooner. This change still kinda eludes me sometimes. Can you clarify:

  1. What would be the meaning of this? What are the scenarios it protects against, and why is it better than what we have right now?
  2. Is this a breaking change? Can it possibly break any projects?
  3. Does it have any impact on Redux DevTools or related tooling? Can you try searching @@redux/INIT in GitHub projects to check that?

Thanks.

@msafi
Copy link
Author

msafi commented Mar 6, 2016

Hey Dan,

  1. So, in the current code, Redux does two checks separately. First, it checks that if undefined state is passed, the reducer knows to return a value. Second, it checks that if unknown action type is passed, the reducer knows to return a value. My change combines these checks, makes the error message clearer, and simplifies the code. It doesn't protect against anything.
  2. I don't see how it could be a breaking change. It's the same functionality, only simplified.
  3. Regarding Redux DevTools, yes, it would break it! And possibly other tooling projects? 😢 To solve this, I could restore PROBE_UNKNOWN_ACTION, and have combineReducers use it to assert reducer sanity.

All in all, this isn't an important change. I just made it as a quick first contribution and it is turning out to be more confusing than it's worth, so feel free to close it! 😄

@timdorr
Copy link
Member

timdorr commented Mar 8, 2016

To solve this, I could restore PROBE_UNKNOWN_ACTION, and have combineReducers use it to assert reducer sanity.

This sounds like the more sane solution to me. Having a separate probe action from the core init action avoids problems when trying to overload its purpose like this.

@gaearon gaearon added this to the 4.0 milestone Jun 11, 2016
@timdorr timdorr closed this Jun 26, 2016
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

Successfully merging this pull request may close these issues.

3 participants