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

Make defaultState required when creating reducers #127

Merged
merged 9 commits into from
Oct 30, 2016

Conversation

yangmillstheory
Copy link
Contributor

@yangmillstheory yangmillstheory commented Sep 5, 2016

Close #18.

See that issue and also reduxjs/redux#514 for background and discussion.


TL;DR

We can prevent downstream reducer sanity errors like this one by throwing earlier.

@yangmillstheory yangmillstheory changed the title Make defaultState parameter required when specifying reducers Make defaultState required when specifying reducers Sep 6, 2016
@yangmillstheory yangmillstheory changed the title Make defaultState required when specifying reducers Make defaultState required when creating reducers Sep 6, 2016
@timche timche mentioned this pull request Sep 6, 2016
3 tasks
@timche timche changed the base branch from master to v1.0.0 September 27, 2016 09:58
@timche
Copy link
Member

timche commented Sep 27, 2016

We are explicitly requiring it, so let's note it as breaking change, regardless of how Redux is handling it.

Copy link
Member

@timche timche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a structural thing, otherwise 👍

`Expected defaultState for reducer handling ${actionTypes.join(', ')} to be defined`
);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to move this function to a folder named utils and name that file assertions.js or sth like this, where we can write more functions like this in a single file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally that's a good idea to extract the assertions 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we will rewrite this later with invariant, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, or we may as well do it now.

@timche
Copy link
Member

timche commented Sep 27, 2016

Btw, I've changed the base branch to v1.0.0, so we can still merge some other stuff into master that's not supposed to be in v1.0.0.

@yangmillstheory
Copy link
Contributor Author

I've updated the PR; it's been a while. We're now using invariant for runtime checks since #141.

@JaKXz @timche can you take another look?

handlers[type],
defaultState
)
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relevant: #108

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants