-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Consider removing DefaultRootState
type
#1879
Comments
The benefits of the pattern are not to be overlooked - you don't need to have all reducers meet in one place, which would be especially desirable in a situation where reducers are injected after the fact. It could, however, easily be recreated in userland: // store/state.ts
export interface MyOwnRootState {}
// store/hooks.ts
import { MyOwnRootState } from './state'
import { TypedUseSelectorHook, useSelector } from 'react-redux'
export const useAppSelector: TypedUseSelectorHook<MyOwnRootState > = useSelector
// feature/something/slice.ts
declare module 'store/state' {
inferface MyOwnRootState {
foo: MyFeatureState
}
} => this does not necessarily need to be shipped with the react-redux types and can be imitated in userland. That way, it's also impossible for random dependencies to poison that type. And it also clearly makes it an "edge case useful in some scenarios" instead of a "first-grade supported" pattern, which I would very much prefer. |
One point where my previous statement would not hold true: But in that light I would suggest we add another type export type TypedConnect<State, Dispatch> = { /* all the current signatures with fixed state and dispatch types */ }
export const connect: TypedConnect<unknown, Dispatch> that |
Makes total sense, even if for no other reason than consistency with how the other redux codebases do things. |
Module Augmentation should be useful in case we want a TS-only solution for typing state. This sounds reasonable because 99% usage of redux would be single store. I kind of liked the solution, but can understand that it may lead to a bad practice. Just my two cents. |
Just did this in #1887 . There was also already a (That |
In case anyone is looking to replicate augmenting the // react-redux.d.ts
import type { EqualityFn } from 'react-redux/es/types';
import type { GlobalState } from '...'; // Your root store type
declare module 'react-redux' {
export declare const useSelector: <T>(selector: (state: GlobalState) => T, equalityFn?: EqualityFn<T>) => T;
} (The file should be placed in a directory listed in the project's Now calls such as |
i got TS error when using this |
@longb1997 Do you have any further details about the error? |
thanks for the reply, this is error: hover on and hover on edit: i am using "typescript": "^4.7.4" |
@longb1997 : note that we specifically advise against trying to do this! Instead, follow our docs guidelines for creating "pre-typed" versions of the hooks: https://redux.js.org/tutorials/typescript-quick-start#define-typed-hooks |
In case someone is looking for a way how to upgrade big projects to react-redux@8 without changing every useSelector/useDispatch import and usage accross all the project to typed version of it (useAppSelector, useAppDispatch), there is my solution (works on typescript project): In your tsconfig.json, add next block:
In react-redux-typed.ts:
That's all. After that, you can leave all useSelector/useDispatch imports and usages as they are, but now they are typed as should be |
React-Redux v8 currently has a
DefaultRootState
type defined:react-redux/src/types.ts
Lines 13 to 23 in 4d38402
This was copy-pasted directly from the
@types/react-redux
types in DefinitelyTyped, which have had that for a while.We Redux maintainers were not involved in that type being added originally, and specifically think it's a bad practice:
Given that, I'd like to propose removing it from the v8 typings before this gets released.
I'm assuming we'd want to replace that with
unknown
instead.I did a couple code searches, and I see maybe 1000-1500 usages in the wild:
So that's not zero, but it isn't a thing that would completely break the ecosystem in the upgrade process.
Lenz also said that this is a pattern that could get replicated in userspace if desired, although I'm not sure what the details are on that.
The text was updated successfully, but these errors were encountered: