fix: remove react-hooks linter suppression #8051
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
No changes to documentation |
|
Component Testing Report Updated Jan 3, 2025 12:48 PM (UTC) ❌ Failed Tests (1) -- expand for details
|
⚡️ Editor Performance ReportUpdated Fri, 03 Jan 2025 12:49:24 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
9d075f4 to
f5cd7df
Compare
7e2a9ac to
e3891b3
Compare
a76e797 to
ff14ce1
Compare
stipsan
commented
Dec 18, 2024
| // Task is updated on every change, wait until the revision changes to update the activity log. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [fetchAndParse, task._rev]) | ||
| handleFetchAndParse(task._rev) |
There was a problem hiding this comment.
Sending the task._rev here is just to make it a valid useEffect dependency in the eyes of eslint-plugin-react-hooks/exhaustive-deps as well as the React Compiler.
stipsan
commented
Dec 18, 2024
Comment on lines
-250
to
-252
| // Explicitly don't include `noDocumentsContent` in the deps array, as it's | ||
| // causing a visual bug where the "No documents" message is shown for a split second | ||
| // when clearing a search query with no results |
There was a problem hiding this comment.
I can't reproduce this issue mentioned here, so I think we're good.
In any case, it's not ideal that what initially was just noDocumentsContent being exempt has eventually led to other deps, t and paneTitle, being accidentally omitted.
It demonstrates why it's risky to suppress the linter to begin with 😅
There was a problem hiding this comment.
Good call, glad to get rid of hacks like this
ff14ce1 to
d5e60d5
Compare
d5e60d5 to
7e05a33
Compare
7e05a33 to
d6aa840
Compare
d6aa840 to
6912ead
Compare
bjoerge
approved these changes
Jan 3, 2025
Comment on lines
-250
to
-252
| // Explicitly don't include `noDocumentsContent` in the deps array, as it's | ||
| // causing a visual bug where the "No documents" message is shown for a split second | ||
| // when clearing a search query with no results |
There was a problem hiding this comment.
Good call, glad to get rid of hacks like this
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Suppressing the linter on
react-hooks/exhaustive-depscauses React Compiler to bail, as it can no longer determine if the optimization it plans to do will preserve the original behavior of how often and whenuseEffecthooks will fire.It's also a bit dangerous to leave in the codebase as what may have started off as omitting a single value from the dependencies array, can over time lead to other values being omitted that would otherwise been caught be the linter.
For example
DocumentListPaneContenthave had its linter suppression for a long long time, and since then we have addedconst {t} = useTranslation(structureLocaleNamespace)andtshould be a dependency, but it's not.This PR removes all of them, and in the case of
useEffecttheuseEffectEventponyfill is used to preserve the original intent of only having the effect fire when specific dependencies have changed, while no longer suppressing the linter.What to review
Is there enough context?
Testing
Existing tests are sufficient?
Notes for release
Removed
react-hooks/exhaustive-depsESLint suppressions, allowing React Compiler to auto-memoize components that were previously skipped over.