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

REPLACE action for replaceReducers #2673

Merged
merged 4 commits into from
Nov 3, 2017
Merged

REPLACE action for replaceReducers #2673

merged 4 commits into from
Nov 3, 2017

Conversation

timdorr
Copy link
Member

@timdorr timdorr commented Oct 22, 2017

Fixes #1636

Provides a separate private action for when we replace reducers. This allows us to specifically handle this case, which resolves a false dev-only warning for combineReducers currently.

@quinnnned
Copy link
Contributor

Hey that test looks familiar. 😉 Glad you liked it.

@quinnnned
Copy link
Contributor

@timdorr what if the nextReducer depends on the INIT action for proper behavior? Or is it just understood that a reducer must not depend on the INIT action?

@timdorr
Copy link
Member Author

timdorr commented Oct 22, 2017

Hey that test looks familiar. 😉 Glad you liked it.

Yes, thank you! I'll amend the commit so you get credit.

what if the nextReducer depends on the INIT action for proper behavior? Or is it just understood that a reducer must not depend on the INIT action?

It's the latter. INIT is a "private" action that shouldn't be relied on outside of the Redux package. We're looking at making it both randomized and more private in 4.0.

@quinnnned
Copy link
Contributor

Gotcha. I also just saw Dan's advice in #186. Should have done a quick google first.

@timdorr
Copy link
Member Author

timdorr commented Oct 23, 2017

As requested in #1736 (comment), this also adds a randomized name to the private actions, making them harder to look for.

INIT: '@@redux/INIT'
const ActionTypes = {
INIT: '@@redux/INIT' + Math.random().toString(36).substring(7).split('').join('.'),
REPLACE: '@@redux/REPLACE' + Math.random().toString(36).substring(7).split('').join('.')

Choose a reason for hiding this comment

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

Does it makes sense to have this random token generator in a function ? (since it being used at least twice here)

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, if we start having more than just two. I'm find with the duplication for now.

Copy link

Choose a reason for hiding this comment

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

Why do you need two separate random strings generated? Seems that you can generate it once and use in both action types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but that seems like splitting hairs. It doesn't really matter.

Copy link

Choose a reason for hiding this comment

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

Ok, fair enough. There's also https://github.com/reactjs/redux/blob/master/src/combineReducers.js#L72. Does it make any sense to put all of them in the same place?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm with @timdorr here. When applying the D.R.Y. principle, it is critical to distinguish between essential duplication and coincidental duplication, because trying to "DRY-up" the latter kind can increase code complexity. In cases where it is not clear what kind of duplication it is, one best practice is to permit the duplication until it causes actual pain. This seems to be what @timdorr is doing.

Copy link
Contributor

@markerikson markerikson Oct 28, 2017

Choose a reason for hiding this comment

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

@timdorr
Copy link
Member Author

timdorr commented Nov 3, 2017

Without further objections, I think this is OK to merge in. If anyone has any issues with it, feel free to revert or submit a PR with improvements.

@timdorr timdorr merged commit 2b90f97 into next Nov 3, 2017
@timdorr timdorr deleted the replace-action branch November 3, 2017 17:24
timdorr added a commit that referenced this pull request Nov 16, 2017
* Add an explicit private action type for replacing reducers

* Make this a const while we're at it...

* Triple equals for lint

* Add a random string to the "private" actions.
seantcoyote pushed a commit to seantcoyote/redux that referenced this pull request Jan 14, 2018
* Add an explicit private action type for replacing reducers

* Make this a const while we're at it...

* Triple equals for lint

* Add a random string to the "private" actions.
tomipaul pushed a commit to tomipaul/redux that referenced this pull request Apr 8, 2018
* Add an explicit private action type for replacing reducers

* Make this a const while we're at it...

* Triple equals for lint

* Add a random string to the "private" actions.
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.

5 participants