-
-
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
Refactor/test hooks #1767
Refactor/test hooks #1767
Conversation
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 8404c7a:
|
✔️ Deploy Preview for react-redux-docs ready! 🔨 Explore the source changes: 8404c7a 🔍 Inspect the deploy log: https://app.netlify.com/sites/react-redux-docs/deploys/60ed79a0448aa4000832c74f 😎 Browse the preview: https://deploy-preview-1767--react-redux-docs.netlify.app |
test/hooks/useSelector.spec.tsx
Outdated
|
||
const Parent = () => { | ||
const { subscription } = useReduxContext() | ||
rootSubscription = subscription | ||
const count = useSelector((s) => s.count) | ||
const count = useSelector<DefaultRootState>((s) => s.count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ Standard usage for useSelector
is to either explicitly declare the type of s
, or create a "pre-typed" version of the hook, per https://redux.js.org/usage/usage-with-typescript#define-typed-hooks .
Either of those is fine with me, but I'd appreciate changing over to those instead of using a generic in all of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a great way to write it. Thank you for your advice
test/hooks/useSelector.spec.tsx
Outdated
@@ -380,20 +406,30 @@ describe('React', () => { | |||
const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) | |||
|
|||
const Parent = () => { | |||
const count = useSelector((s) => s.count) | |||
const count = useSelector<DefaultRootState, number>((s) => s.count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ Similarly, here the return type should be inferred correctly if the state is typed right
interface ParentContentProps { | ||
parentCount: number | ||
} | ||
const ConnectedWrapper = connect< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 here on the other hand, connect
mostly requires the generics
Looks good! |
add ts-jest and modify jest config
transform test/hooks js files to tsx
ref: #1737