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

Update createReducer to accept a lazy state init function #1662

Merged
merged 4 commits into from
Oct 29, 2021
Merged

Update createReducer to accept a lazy state init function #1662

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e6a1d48
Update createReducer to accept a lazy state init function
markerikson Oct 28, 2021
fd31cdc
Update createSlice to accept a lazy init function
markerikson Oct 28, 2021
010bcf0
Ensure initial state gets frozen
markerikson Oct 29, 2021
eaca9de
Document lazy state init option
markerikson Oct 29, 2021
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ temp/
.tmp-projections
build/
.rts2*
coverage/

typesversions
.cache
Expand Down
42 changes: 30 additions & 12 deletions packages/toolkit/src/createReducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,16 @@ export type CaseReducers<S, AS extends Actions> = {
[T in keyof AS]: AS[T] extends Action ? CaseReducer<S, AS[T]> : void
}

export type NotFunction<T> = T extends Function ? never : T

function isStateFunction<S>(x: unknown): x is () => S {
return typeof x === 'function'
}

export type ReducerWithInitialState<S extends NotFunction<any>> = Reducer<S> & {
getInitialState: () => S
}

/**
* A utility function that allows defining a reducer as a mapping from action
* type to *case reducer* functions that handle these action types. The
Expand Down Expand Up @@ -130,10 +140,10 @@ createReducer(
```
* @public
*/
export function createReducer<S>(
initialState: S,
export function createReducer<S extends NotFunction<any>>(
initialState: S | (() => S),
builderCallback: (builder: ActionReducerMapBuilder<S>) => void
): Reducer<S>
): ReducerWithInitialState<S>

/**
* A utility function that allows defining a reducer as a mapping from action
Expand All @@ -151,7 +161,7 @@ export function createReducer<S>(
* This overload accepts an object where the keys are string action types, and the values
* are case reducer functions to handle those action types.
*
* @param initialState - The initial state that should be used when the reducer is called the first time.
* @param initialState - The initial state that should be used when the reducer is called the first time. This may optionally be a "lazy state initializer" that returns the intended initial state value when called.
* @param actionsMap - An object mapping from action types to _case reducers_, each of which handles one specific action type.
* @param actionMatchers - An array of matcher definitions in the form `{matcher, reducer}`.
* All matching reducers will be executed in order, independently if a case reducer matched or not.
Expand Down Expand Up @@ -180,31 +190,35 @@ const counterReducer = createReducer(0, {
* @public
*/
export function createReducer<
S,
S extends NotFunction<any>,
CR extends CaseReducers<S, any> = CaseReducers<S, any>
>(
initialState: S,
initialState: S | (() => S),
actionsMap: CR,
actionMatchers?: ActionMatcherDescriptionCollection<S>,
defaultCaseReducer?: CaseReducer<S>
): Reducer<S>
): ReducerWithInitialState<S>

export function createReducer<S>(
initialState: S,
export function createReducer<S extends NotFunction<any>>(
initialState: S | (() => S),
mapOrBuilderCallback:
| CaseReducers<S, any>
| ((builder: ActionReducerMapBuilder<S>) => void),
actionMatchers: ReadonlyActionMatcherDescriptionCollection<S> = [],
defaultCaseReducer?: CaseReducer<S>
): Reducer<S> {
): ReducerWithInitialState<S> {
let [actionsMap, finalActionMatchers, finalDefaultCaseReducer] =
typeof mapOrBuilderCallback === 'function'
? executeReducerBuilderCallback(mapOrBuilderCallback)
: [mapOrBuilderCallback, actionMatchers, defaultCaseReducer]

const frozenInitialState = createNextState(initialState, () => {})
const getInitialState = () =>
createNextState(
isStateFunction(initialState) ? initialState() : initialState,
() => {}
)
Copy link
Member

@phryneas phryneas Oct 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This would be my suggestion to be a bit more bullet-proof there

Suggested change
const getInitialState = () =>
createNextState(
isStateFunction(initialState) ? initialState() : initialState,
() => {}
)
let getInitialState: () => S
if (isStateFunction(initialState)) {
getInitialState = initialState
} else {
const frozenInitialState = createNextState(initialState, () => {})
getInitialState = () => frozenInitialState
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically that doesn't prevent a user-provided initialState() function from returning a non-frozen value / mutable, I think

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, the call to getInitialState would not be frozen then. Yeah, that needs another createNextState call then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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
  }

Copy link
Member

Choose a reason for hiding this comment

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

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.


return function (state = frozenInitialState, action): S {
function reducer(state = getInitialState(), action: any): S {
let caseReducers = [
actionsMap[action.type],
...finalActionMatchers
Expand Down Expand Up @@ -257,4 +271,8 @@ export function createReducer<S>(
return previousState
}, state)
}

reducer.getInitialState = getInitialState

return reducer as ReducerWithInitialState<S>
}
13 changes: 11 additions & 2 deletions packages/toolkit/src/createSlice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type {
} from './createAction'
import { createAction } from './createAction'
import type { CaseReducer, CaseReducers } from './createReducer'
import { createReducer } from './createReducer'
import { createReducer, NotFunction } from './createReducer'
import type { ActionReducerMapBuilder } from './mapBuilders'
import { executeReducerBuilderCallback } from './mapBuilders'
import type { NoInfer } from './tsHelpers'
Expand Down Expand Up @@ -53,6 +53,12 @@ export interface Slice<
* This enables reuse and testing if they were defined inline when calling `createSlice`.
*/
caseReducers: SliceDefinedCaseReducers<CaseReducers>

/**
* Provides access to the initial state value given to the slice.
* If a lazy state initializer was provided, it will be called and a fresh value returned.
*/
getInitialState: () => State
}

/**
Expand All @@ -72,8 +78,10 @@ export interface CreateSliceOptions<

/**
* The initial state to be returned by the slice reducer.
* This may optionally be a "lazy state initializer" that returns the
* intended initial state value when called.
*/
initialState: State
initialState: State | (() => State)

/**
* A mapping from action types to action-type-specific *case reducer*
Expand Down Expand Up @@ -301,5 +309,6 @@ export function createSlice<
reducer,
actions: actionCreators as any,
caseReducers: sliceCaseReducersByName as any,
getInitialState: reducer.getInitialState,
}
}
39 changes: 38 additions & 1 deletion packages/toolkit/src/tests/createReducer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,10 @@ describe('createReducer', () => {
test('Freezes initial state', () => {
const initialState = [{ text: 'Buy milk' }]
const todosReducer = createReducer(initialState, {})
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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
  • createReducer captures that value for later
  • Slice module finishes executing, as do all other store dependency modules
  • body of store.js now executes
  • configureStore runs, dispatches '@@init', combineReducers does 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.

Copy link
Member

@phryneas phryneas Oct 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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 frozenInitialState = todosReducer(undefined, { type: 'dummy' })

const mutateStateOutsideReducer = () => (initialState[0].text = 'edited')
const mutateStateOutsideReducer = () =>
(frozenInitialState[0].text = 'edited')
expect(mutateStateOutsideReducer).toThrowError(
/Cannot assign to read only property/
)
Expand Down Expand Up @@ -132,6 +134,41 @@ describe('createReducer', () => {
behavesLikeReducer(todosReducer)
})

describe('Accepts a lazy state init function to generate initial state', () => {
const addTodo: AddTodoReducer = (state, action) => {
const { newTodo } = action.payload
state.push({ ...newTodo, completed: false })
}

const toggleTodo: ToggleTodoReducer = (state, action) => {
const { index } = action.payload
const todo = state[index]
todo.completed = !todo.completed
}

const lazyStateInit = () => [] as TodoState

const todosReducer = createReducer(lazyStateInit, {
ADD_TODO: addTodo,
TOGGLE_TODO: toggleTodo,
})

behavesLikeReducer(todosReducer)

it('Should only call the init function when `undefined` state is passed in', () => {
const spy = jest.fn().mockReturnValue(42)

const dummyReducer = createReducer(spy, {})
expect(spy).not.toHaveBeenCalled()

dummyReducer(123, { type: 'dummy' })
expect(spy).not.toHaveBeenCalled()

const initialState = dummyReducer(undefined, { type: 'dummy' })
expect(spy).toHaveBeenCalledTimes(1)
})
})

describe('given draft state from immer', () => {
const addTodo: AddTodoReducer = (state, action) => {
const { newTodo } = action.payload
Expand Down
42 changes: 42 additions & 0 deletions packages/toolkit/src/tests/createSlice.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,48 @@ describe('createSlice', () => {
expect(caseReducers.increment).toBeTruthy()
expect(typeof caseReducers.increment).toBe('function')
})

it('getInitialState should return the state', () => {
const initialState = 42
const slice = createSlice({
name: 'counter',
initialState,
reducers: {},
})

expect(slice.getInitialState()).toBe(initialState)
})
})

describe('when initialState is a function', () => {
const initialState = () => ({ user: '' })

const { actions, reducer } = createSlice({
reducers: {
setUserName: (state, action) => {
state.user = action.payload
},
},
initialState,
name: 'user',
})

it('should set the username', () => {
expect(reducer(undefined, actions.setUserName('eric'))).toEqual({
user: 'eric',
})
})

it('getInitialState should return the state', () => {
const initialState = () => 42
const slice = createSlice({
name: 'counter',
initialState,
reducers: {},
})

expect(slice.getInitialState()).toBe(42)
})
})

describe('when mutating state object', () => {
Expand Down