-
-
Notifications
You must be signed in to change notification settings - Fork 208
Reduce react warnings #5770
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
base: main
Are you sure you want to change the base?
Reduce react warnings #5770
Conversation
|
@WofWca we have 7 warnings just from the ChatlistContext file. Maybe you can have a look how to fix/avoid them? |
WofWca
left a comment
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.
Although some of these can probably be fixed instead of simply being ignored, I think it's fine to ignore them if there is no obvious problem that they're pointing out. The fact that a warning is ignored is also sort of a warning.
disabling the eslint rule for react-hooks/refs on places where it seems the warning is inappropriate
9344f05 to
ef0b895
Compare
| const [currentTime] = useState(() => Date.now()) | ||
|
|
||
| // if older than one week we don't need to update timestamps | ||
| if (props.timestamp < Date.now() - relativeTimeThreshold) { | ||
| const isOld = useMemo( | ||
| () => props.timestamp < currentTime - relativeTimeThreshold, | ||
| [props.timestamp, currentTime] | ||
| ) |
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.
Utilizing useState over useMemo for currentTime seems to be just a hack to suppress the warning here or am I wrong? Let's better suppress it explicitly.
| const [currentTime] = useState(() => Date.now()) | |
| // if older than one week we don't need to update timestamps | |
| if (props.timestamp < Date.now() - relativeTimeThreshold) { | |
| const isOld = useMemo( | |
| () => props.timestamp < currentTime - relativeTimeThreshold, | |
| [props.timestamp, currentTime] | |
| ) | |
| // if older than one week we don't need to update timestamps | |
| const isOld = useMemo( | |
| // eslint-disable-next-line react-hooks/purity | |
| () => props.timestamp < Date.now() - relativeTimeThreshold, | |
| [props.timestamp] | |
| ) |
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.
Both variants seem fine to me
357d30e to
f4af6e7
Compare
WofWca
left a comment
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.
The misuse of useState is not nice, but otherwise looks good (see my previous review).
Reduces the number of warnings from 37 to 16. Many of the warnings seem inappropriate so just adding a line to disable the rook was often used.
Can be best reviewed commit by commit.