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

Still return defaultState in handleActions reducer when no handlers are provided #203

Merged
merged 9 commits into from
Apr 16, 2017

Conversation

webholics
Copy link
Contributor

Fixes #202

@yangmillstheory
Copy link
Contributor

I think I misread #202 (comment).

Why do you think an empty object is not an usage error? I'd have thought that we would be throwing an error (using invariant) and writing a test for that instead.

@webholics
Copy link
Contributor Author

First: It was possible in previous versions of redux-actions. So it was a breaking change for us.
For sure it is an edge case.
But my argument would be that it is still semantically correct to give the handleActions function an empty object which would mean "please do not handle any actions (at the moment)". Returning the default state doesn't have anything to do with how many actions the reducer handles (0-infinity) and should always work.

@yangmillstheory
Copy link
Contributor

Thanks for the reply. We're on 2.0 now, that breaking change should have been noted. Though I'm not sure when this break occurred.

I can't think of a use case in real apps. Could you give an example?

@webholics
Copy link
Contributor Author

webholics commented Mar 21, 2017

You're right there is no "real" use case for that. However we often start new redux/react apps with an empty skeleton of different reducers and this breaks our skeletons now during development.
Its not a huge deal. I would also be fine with an invariant check, but it shouldn't just silently break as it does now.

@yangmillstheory
Copy link
Contributor

yangmillstheory commented Mar 21, 2017

I think defaulting to defaultState is still correct for the created reducer. I'm wondering if this would also fix #197.

Can we do an invariant check in handleActions (maybe using isPlainObject), write some tests for the new API and defaultState behavior (with a valid action), and we can get this merged and released? Thanks for your time.

@webholics
Copy link
Contributor Author

I update the PR with more tests and the invariant check. Hope the expected behaviour is better described now.

Regarding #197
It is not fixed yet with this PR. I think the origin of this issue is that the defaultState should not be forwarded to the handleAction calls at all. handleAction should be called with an undefined defaultState in this case. Let the handleActions handle setting the default itself as it is now implemented here. This of course would require the removal of the invariant check of defaultState in handleAction. Which for me would be OK because it is totally valid to have an undefined defaultState.

@yangmillstheory yangmillstheory self-requested a review April 16, 2017 21:06
@yangmillstheory
Copy link
Contributor

Thanks for the contribution!

@yangmillstheory yangmillstheory merged commit feb9386 into redux-utilities:master Apr 16, 2017
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.

2 participants