-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: react-hooks/exhaustive-deps warnings #886
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
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 22a1bc1:
|
d9e3ec5
to
986826e
Compare
src/use-swr.ts
Outdated
rerender({}) | ||
} | ||
}, | ||
[config.suspense] |
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.
We need to ignore this dependence because we don't allow the suspense
option to change during the lifecycle, it has to be static (otherwise it would be difficult to track the internal state).
Maybe we should warn the user when they try to change the option.
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.
@shuding Thank you for your feedback. I've disabled the warning by the eslint-disable comment instead of adding it to the deps array.
Maybe we should warn the user when they try to change the option.
That's a good idea. I think the warning isn't required on a production environment, but swr
doesn't have a way to print warnings only for a development build. I'll propose it later.
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.
Thank you!
Thank you! |
This is based on #263 and I've fixed some warnings of
react-hooks/exhaustive-deps
, which can be safely fixed.renderer
is a setState function thatuseState
returns, which is guaranteed to be stable. But the eslint plugin cannot recognize thatconst rerender = useState<boolean>(false)[1]
is the setState function, so I've changed the statement.dispatch
function usesconfig.suspense
, but it's not in the deps array, so I've added it to the deps array.This PR doesn't fix all warnings because some of them seem to be valid and adding missing deps variables might cause issues, so we have to fix the issues carefully and consider adding missing deps variables or
eslint-disable
comments. I'll work on that after this has been merged.