Update createReducer to accept a lazy state init function #1662
Conversation
size-limit report 📦
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit eaca9de:
|
| @@ -98,8 +98,10 @@ describe('createReducer', () => { | |||
| test('Freezes initial state', () => { | |||
| const initialState = [{ text: 'Buy milk' }] | |||
| const todosReducer = createReducer(initialState, {}) | |||
There was a problem hiding this comment.
I think this now shows a valid concern with the lazyness. Imagine this situation:
const initialState = { foo: "bar" }
const reducer = createReducer(initialState, {})
// foo: "bar"
console.log(reducer(undefined, { type: "init"}))
console.log(reducer.getInitialState())
initialState.foo = "baz"
// foo: "baz" - should not happen
console.log(reducer(undefined, { type: "init"}))
console.log(reducer.getInitialState())
so with a non-function being passed in, we still need to create a frozen copy of the item inside createReducer - but without freezing the outside initialState variable as was the behaviour before. That seems kinda dangerous, too.
There was a problem hiding this comment.
Hmm. I agree there's some change in semantics here. On the other hand... the idea of the original state being mutated between the creation of the slice and the store being initialized seems almost nonexistent.
Currently, what would happen is the initial state object gets frozen synchronously during the initial module import processing sequence, before any of the code in store.js would have a chance to run.
If we switch to this, you'd have:
- Initial object created
createReducercaptures that value for later- Slice module finishes executing, as do all other store dependency modules
- body of
store.jsnow executes configureStoreruns, dispatches'@@init',combineReducersdoes its things, and now we freeze and return the initial state.
So we're talking the same synchronous execution sequence, just moving the freezing to be somewhat later
I suppose there's other possible edge cases, like using createSlice with useReducer, where there's more of a lag time between the time the reducer captures the initial value and when it's actually used. But even then I see mutation of the initial state as a relatively rare concern.
There was a problem hiding this comment.
I think this could for example play a role in tests or situations where people accidentally create a new store on every render and just keep modifying initialState.
There are many questions on StackOverflow that have "how to change initialState" in the title, which shows that many people really have the concept of "changing initial state outside the reducer" in mind - so I'd like to be as bullet-proof as possible here, even if it means shipping a few extra bytes.
| const getInitialState = () => | ||
| createNextState( | ||
| isStateFunction(initialState) ? initialState() : initialState, | ||
| () => {} | ||
| ) |
There was a problem hiding this comment.
This would be my suggestion to be a bit more bullet-proof there
| const getInitialState = () => | |
| createNextState( | |
| isStateFunction(initialState) ? initialState() : initialState, | |
| () => {} | |
| ) | |
| let getInitialState: () => S | |
| if (isStateFunction(initialState)) { | |
| getInitialState = initialState | |
| } else { | |
| const frozenInitialState = createNextState(initialState, () => {}) | |
| getInitialState = () => frozenInitialState | |
| } |
There was a problem hiding this comment.
Technically that doesn't prevent a user-provided initialState() function from returning a non-frozen value / mutable, I think
There was a problem hiding this comment.
Ah, right, the call to getInitialState would not be frozen then. Yeah, that needs another createNextState call then
There was a problem hiding this comment.
But, we'd have to defer the createNextState call until usage time, in which case the external state could still get mutated in the meantime?
Still feels like we're trying to solve a problem that's outside our scope here, tbh.
Here's what I've got atm:
let getInitialState: () => S
if (isStateFunction(initialState)) {
getInitialState = () => createNextState(initialState(), () => {})
} else {
const frozenInitialState = createNextState(initialState, () => {})
getInitialState = () => frozenInitialState
}There was a problem hiding this comment.
If someone willingly provides a "lazy initializer" function, I guess they expect that it returns the value at the time of call, not something from before, so I'd say that now it looks like exactly the expected behaviour.
This PR:
createReducerto accept a lazy state initialization function that won't be called until the reducer is called withundefined.getInitialStatefunction to the generated reducer, which will either return the original state value or call the lazy state init functioncreateSliceto accept the same type signaturecreateSliceto expose the reducer's.getInitialStatemethod as part of the slice objectFixes #1024 .
(Thanks to @JoshuaKGoldberg for the assist with the "is this a function?" check!)