Skip to content

refactor: fix all react-hooks/exhaustive-deps warnings in useSWR #927

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

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

koba04
Copy link
Collaborator

@koba04 koba04 commented Jan 22, 2021

This is the last PR to fix all react-hooks/exhaustive-deps warnings.
The PR fixes the warnings in useSWR, which adds some deps for memoizedState and disables the rule for revalidate.

For memoizedState, I've added all deps in the deps array. boundMutate is an immutable function, so we can omit it, but I've added it to the deps array to enable react-hooks/exhaustive-deps rule. It's an immutable function, so it shouldn't be a problem.

For revalidate, I've disabled the react-hooks/exhaustive-deps rule because fn and config are defined as the arguments of useSWR call, which means those values would be changed every time.
I could add eventsCallback, fnArgs, keyErr, and keyValidating into the deps array. It should be safe because they only depend on key that is already in the deps array.
But it makes tests flakier because current tests depends on JavaScript timer naively. If I added meaning-less values in the deps array for revalidate, tests would be failed frequently. So I didn't add them into the deps array.

[key, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]

I'll investigate the way to fix flaky tests.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 22, 2021

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 397339f:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration

@koba04 koba04 force-pushed the fix-all-exhaustive-deps-warnings branch from fd10812 to 397339f Compare January 23, 2021 08:25
@koba04 koba04 marked this pull request as ready for review January 23, 2021 09:28
Comment on lines +555 to +560
// FIXME:
// `fn` and `config` might be changed during the lifecycle,
// but they might be changed every render like this.
// useSWR('key', () => fetch('/api/'), { suspense: true })
// So we omit the values from the deps array
// even though it might cause unexpected behaviors.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is expected, I'm okay with the eslint ignore mark here. 👍

@shuding
Copy link
Member

shuding commented Jan 26, 2021

Thanks!

@shuding shuding merged commit 369266d into vercel:master Jan 26, 2021
@koba04
Copy link
Collaborator Author

koba04 commented Jan 27, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants