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

Describe inferring RootState type from store.getState #324

Closed
markerikson opened this issue Jan 20, 2020 · 29 comments · Fixed by #326
Closed

Describe inferring RootState type from store.getState #324

markerikson opened this issue Jan 20, 2020 · 29 comments · Fixed by #326
Labels
docs good first issue Good for newcomers

Comments

@markerikson
Copy link
Collaborator

markerikson commented Jan 20, 2020

The docs show inferring type RootState = ReturnType<typeof rootReducer>. But, if you pass the slice reducers directly to configureStore(), you never created the root reducer yourself, so you need to infer it as type RootState = ReturnType<typeof store.getState> instead.

We should update the Advanced Tutorial and Usage with TypeScript pages to mention that.

See https://stackoverflow.com/a/59827580/62937 for an example of this

@markerikson markerikson added docs good first issue Good for newcomers labels Jan 20, 2020
@firstred
Copy link
Contributor

firstred commented Jan 21, 2020

That works indeed! I just arrived at that section of the documentation this morning. Here are my results with TypeScript 3.7.5:

It fails with combineReducers() or passing them directly to configureStore(). The type of RootState would always be unknown.
The only thing that works is, indeed, using type RootState = ReturnType<typeof store.getState> and passing the reducers directly to configureStore(). combineReducers() would still turn the type into unknown.

Based on my experience, I would recommend to leave out combineReducers() in the TypeScript documentation until a solution for combineReducers() has been found. Until then passing the reducers directly to configureStore() and using ReturnType<typeof store.getState> would be a nice alternative.

BTW: Redux Toolkit has been really helpful so far. Thanks a lot for another massive time-saver!

@markerikson
Copy link
Collaborator Author

markerikson commented Jan 21, 2020

@firstred : ah.... I'm very confused. Inferring the return type of combineReducers does work, as I showed in the Advanced Tutorial. See this sandbox:

https://codesandbox.io/s/rtk-github-issues-example-03-final-ihttc?fontsize=14&hidenavigation=1&module=%2Fsrc%2Fapp%2FrootReducer.ts&theme=dark

If I hover over export type RootState = ReturnType<typeof rootReducer>, I see:

const rootReducer: Reducer<CombinedState<{
    issuesDisplay: CurrentDisplayState;
    repoDetails: RepoDetailsState;
    issues: IssuesState;
    comments: CommentsState;
}>, AnyAction>

Where and how are you seeing problems with this?

@firstred
Copy link
Contributor

PhpStorm 2019.3.1 + TypeScript 3.7.5. This is what I am seeing:

typeof rootReducer + combineReducers():
rootreducer

typeof store.getState + combineReducers():
getstate

typeof rootReducer:
rootreducernotcombined

typeof store.getState:
getstatenotcombined

Somehow combineReducers() has a return type of Reducer<unknown>.
combinereducersreturntype

I did use createAction() to create the actions. Maybe that could be the issue.

@markerikson
Copy link
Collaborator Author

Can you put together a project that reproduces this, either as a CodeSandbox or a separate repo?

@firstred
Copy link
Contributor

Found the issue!

I hadn't understood from the documentation that I was supposed to import combineReducers() from @reduxjs/toolkit and not from redux. PhpStorm somehow only suggests the one from redux.

Back the issues described in the OP: that works for me now. How about improving these particular examples some more by showing that you have to import from @reduxjs/toolkit instead of redux? I'll try to make a PR :)

@markerikson
Copy link
Collaborator Author

Hmm. It's the exact same function - RTK just does export {combineReducers} from "redux".

There shouldn't be any actual difference in behavior or types at all.

@firstred
Copy link
Contributor

Mind = blown haha!

@firstred
Copy link
Contributor

Indeed, it still works: https://codesandbox.io/s/rtk-github-issues-example-03-final-co03n

I am unable to import the specific commit into CodeSandbox, but here's a link to it: https://github.com/firstred/bunqDesktop/blob/f514eb8f60e1f26cc997f4befaa9c51a4f65e422/src/store/index.ts
It is part of a pull request trying to migrate to Redux Toolkit. Both VS Code and PhpStorm are having problems with these specific changes and only when you import combineReducers() from redux. Just wrap the reducer at line 79 with combineReducers and see how @reduxjs/toolkit and redux show different behavior. Strange...
I'll try to reproduce this on another system. Maybe it's just this particular laptop.

@phryneas
Copy link
Member

Maybe you have an older version of redux installed than the one re-exported from RTK?

@markerikson
Copy link
Collaborator Author

markerikson commented Jan 21, 2020

Yep, I bet that's it.

Looking at package.json from that repo, I see "@reduxjs/toolkit": "^1.2.3", under dependencies, and also "redux": "^3.7.2", under devDependencies.

That would likely end up with both redux@4 and redux@3 installed somehow.

@firstred
Copy link
Contributor

firstred commented Jan 21, 2020

Ahh, good catch! That was the issue. Didn't realize RTK needed 4.0.0 or higher.

Thanks everyone!

@markerikson
Copy link
Collaborator Author

All that said, I'd still like to have the original described RootState bit added to the docs :)

@firstred
Copy link
Contributor

firstred commented Jan 21, 2020

Yes, back to the original issue. I now finally have the right tools installed to confirm that it is indeed better if you prefer use configureStore() directly :)

I'll create a PR!

@amankkg
Copy link

amankkg commented Feb 7, 2020

Such changes would conflict with this advice from docs on getDefaultMiddleware<RootState> https://redux-starter-kit.js.org/usage/usage-with-typescript#correct-typings-for-the-dispatch-type

const store = configureStore({
  reducer: rootReducer,
  middleware: [
    ...customMiddleware,
    ...getDefaultMiddleware<RootState>(),
  ] as const,
})

// Type alias 'RootState' circularly references itself. ts(2456)
export type RootState = ReturnType<typeof store.getState>

@firstred
Copy link
Contributor

Indeed, thanks for pointing that out @amankkg!

We should probably advise against passing the reducers directly to configureStore() when you add custom middleware. There doesn't seem to be a way around the circular reference once you rely on getDefaultMiddleware AND try to derive the return type of store.getState directly in the example shown above.

@firstred
Copy link
Contributor

firstred commented Feb 10, 2020

I have added a warning: fb5dd97

@lucassus
Copy link

Recently I was struggling with circular references problem in my code.
In summary store.tsx exports RootState type that is inferred from store.getState. I need RootState type in store/counter/actions.tsx module which is required by the todos reducer, rootReducer and finally store.tsx which defines it.

I have read an article that proposes a solution https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de but TBH in my opinion a tick with "internal" module could add a lot of noise to the code.

The only reasonable solution I have found so far is to move type definitions to a separate module, for example store/types.ts and define RootState in a more explicit way, like:

types.tx

export type RootState = {
  todos: Todo[];
};

...which IMHO is not that bad because it works like kind of documentation that clearly describes a shape of the app state.

In the "sub-reducers" RootState type it could be used like:

todos/reducer.tsx

const initialState: RootState["todos"] = [];

export const reducer = createReducer(initialState, ....);

@markerikson
Is there a better pattern that could fix the problem?
If not, maybe it would be a good idea to add a note about "explicit RootState type" in the documentation?

Some exemplary code that illustrates the problem

store.ts

import { rootReducer } from "./store/rootReducer";

export store = createStore({
  reducer: rootReducer
});

export RootState = ReturnType<typeof store.getState>;

export type AppThunk = ThunkAction<void, RootState, unknown, Action<string>>;

store/rootReducer.ts

import { reducer as todosReducer } from "./todos/reducer";

export rootReducer = combineReducers({
  todos: todosReducer
})

store/todos/reducer.ts

const initialState = [];

export reducer = createReducer(initialState, builder =>
  builder
    .addCase(fetchTodos.fulfilled, (draft, { payload: todos }) => todos)
    .addCase(updateTodo.fultilled, (draft, { payload: { id, tod } }) => [...])
);

store/counter/actions.ts

import { AppThunk } from '../store'; // TODO: this import causes circular reference problem

export const updateTodo = createAsybcThunk(...);

export const completeAll = (): AppThunk => (dispatch, getState) => {
  return Promise.all(getState().todos.map(todo => {
    return dispatch(updateTodo({ id: todo.id, { ...todo, completed: true } }));
  }));
}

@phryneas
Copy link
Member

Doing

export RootState = ReturnType<typeof rootReducer>;

should break your circular reference more early.

For TS, it doesn't so much matter if the modules depend on each other. So importing files in a circle is no problem. It only matters when your actual types depend on each other circularly.

@markerikson
Copy link
Collaborator Author

Yep. The TS compiler can handle circular references fine. Export a slice reducer, import into the root reducer, export the root state type, import the state type into the slice, and it works fine. It's actual runtime circular references that cause problems.

@lucassus
Copy link

lucassus commented Apr 9, 2020

Got it, thank you for the explanation! You're right, in my case it wasn't a problem with circular imports.
I managed to find the root of the problem by reducing my code to the following example - https://codesandbox.io/s/funny-boyd-jo544?file=/src/store.ts

Here everything is fine but as soon as I add RootState typing for state in selectCounterValue = (state) => {...} the compiler starts to complain.

image

It happens because selectCounterValue is used in multiplyBy sync thunk. I know that it's a silly example because the reducer could take this responsibility but in real-life projects accessing the state in a thunk could be quite reasonable.

Unfortunately the same problem strikes again if I remove the selector from the thunk and manually specify store's type in async thunk config, like:

export const multiplyBy = createAsyncThunk<
  number,
  number,
  {
    state: RootState;
  }
>("counter/multiplyBy", (by, { getState }) => {
  const value = getState().counter.value;
  return value * by;
});

I'm not a TypeScript expert but I guess that it happens because RootState depends on rootReducer, which depends on the slice, which uses thunk that uses a selector (or has RootState typing).

So I'm going to raise the same question again - Is there a workaround for this?

@AndrewCraswell
Copy link
Contributor

Having gone through the docs recently myself, I think it could be useful to mention how to properly get the typings for state when doing code-splitting with dynamically loaded reducers, using replaceReducer. There is mention of code-splitting, but not how it affects the typing of the root state.

@msutkowski
Copy link
Member

@lucassus Here's a forked codesandbox from yours utilizing both methods shown in the docs to define RootState: https://codesandbox.io/s/silent-cloud-jk8bm?file=/src/store.ts:443-476

@lucassus
Copy link

lucassus commented Apr 17, 2020

@msutkowski Thank you very much! You saved me quite a few hours of debugging ;)
It seems that in my case it was a problem with the reducer builder. For example instead of builder => builder.addCase(...), I should have builder => { builder.addCase(...) }.

@markerikson Why in this case these brackets are so important?
Is the typing system somehow relies on the return value form the builder?

@calvinwyoung
Copy link

Per @lucassus, using builder => { builder.addCase() } instead of builder => builder.addCase() fixed the circular dependency issue for me as well.

This is pretty subtle and unintuitive. It's also unfortunate that some of the examples on the Usage with Typescript page use the builder => builder.addCase() syntax.

@msutkowski Would it be worth updating the examples on that page, and explicitly mentioning this specific behavior?

@vcavallo
Copy link

vcavallo commented Apr 23, 2021

Switching the builder syntax doesn't resolve the circular reference warning for me, but

#324 (comment) 's

export type RootState = ReturnType<typeof rootReducer>;

Does.

I'm using configureStore like this:

// using combineReducers
const allReducers = combineReducers({
  auth: AuthReducer,
})

// a persistence library
const persistedReducer = persistReducer(persistConfig, allReducers);

import { someMiddleware } from '../middleware/something';

const customizedMiddleware = getDefaultMiddleware({
  serializableCheck: {
    ignoredActions: [some, things, here],
  }
}).concat(
  someMiddleware,
)

const store = configureStore({
  reducer: persistedReducer, // the combined reducer
  middleware: customizedMiddleware,
})

// Doesn't complain:
export type RootState = ReturnType<typeof persistedReducer>

// Circular reference complaint:
// export type RootState = ReturnType<typeof store.getState>


/// --- in a slice somewhere ---

import type { RootState } from '../store/store'

const someSlice = createSlice({
  // ...
  extraReducers: builder => {
    builder.addCase(LogIn.fulfilled, (state, action: PayloadAction<any>) => {
      // ...
    })
  }
})

@markerikson
Copy link
Collaborator Author

Can someone summarize the changes that would be useful at this point?

Sounds like one thing might be to use the ES6 inline object syntax (extraReducers() {}) instead of an arrow function ( extraReducers: () => {}), to avoid the implicit return issue.

@dmitryshelomanov
Copy link

any solution to fix circularly references ?

@phryneas
Copy link
Member

phryneas commented Dec 5, 2021

@dmitryshelomanov this is extremely specific regarding the individual code. Some examples were given in this issue, but nobody will be able to tell you what works for you without you either trying it out or giving a reasonable reproduction.

@DercilioFontes
Copy link

I was having export type RootState = ReturnType<typeof store.getState>; getting the type as any. So, it was useless. I was adding the types later as "something". But It was annoying to have to add the slice types everywhere.

However, I found that passing the reducers (from combineReducers) to the persistReducer was making it lose the types to any. So, adding <ReturnType<typeof reducers>> to it keeps the slice types.

const persistedReducer = persistReducer<ReturnType<typeof reducers>>(persistConfig, reducers);

Before:
Screen Shot 2022-01-04 at 11 18 34 AM

Later:
Screen Shot 2022-01-04 at 11 18 46 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.