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.

Sign up for GitHub

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

Jump to bottom

REPLACE action for replaceReducers #2673

Merged
merged 4 commits into from
Nov 3, 2017
Merged

REPLACE action for replaceReducers #2673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
569791c
Add an explicit private action type for replacing reducers
timdorr Oct 22, 2017
89f91c0
Make this a const while we're at it...
timdorr Oct 22, 2017
061bffa
Triple equals for lint
timdorr Oct 22, 2017
e54a7f2
Add a random string to the "private" actions.
timdorr Oct 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/combineReducers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ function getUnexpectedStateShapeWarningMessage(inputState, reducers, action, une
unexpectedKeyCache[key] = true
})

if (action && action.type === ActionTypes.REPLACE) return

if (unexpectedKeys.length > 0) {
return (
`Unexpected ${unexpectedKeys.length > 1 ? 'keys' : 'key'} ` +
Expand Down
2 changes: 1 addition & 1 deletion src/createStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ export default function createStore(reducer, preloadedState, enhancer) {
}

currentReducer = nextReducer
dispatch({ type: ActionTypes.INIT })
dispatch({ type: ActionTypes.REPLACE })
}

/**
Expand Down
5 changes: 3 additions & 2 deletions src/utils/actionTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
* If the current state is undefined, you must return the initial state.
* Do not reference these action types directly in your code.
*/
var ActionTypes = {
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('.')
Copy link

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 •
edited
Loading

Choose a reason for hiding this comment

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

@Quinned : highly relevant here: https://twitter.com/acemarke/status/901329101088215044 :) Also, https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction .

}

export default ActionTypes
26 changes: 26 additions & 0 deletions test/createStore.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,4 +767,30 @@ describe('createStore', () => {
expect(results).toEqual([ { foo: 0, bar: 0, fromRx: true }, { foo: 1, bar: 0, fromRx: true } ])
})
})

it('does not log an error if parts of the current state will be ignored by a nextReducer using combineReducers', () => {
const originalConsoleError = console.error
console.error = jest.fn()

const store = createStore(
combineReducers({
x: (s=0, a) => s,
y: combineReducers({
z: (s=0, a) => s,
w: (s=0, a) => s,
}),
})
)

store.replaceReducer(
combineReducers({
y: combineReducers({
z: (s=0, a) => s,
}),
})
)

expect(console.error.mock.calls.length).toBe(0)
console.error = originalConsoleError
})
})