[core] Add eslint-plugin-react-hooks #14629
Merged
Conversation
This file contains hidden or 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
Arguable if this is actual linter appeasment. The statement about is not valueable. effects are not exectured on the server anyway so there is no performance gain. only noise in the code
window.matchMedia was called on every render for defaultValue. We now only call it on state initialization.
Makes for cleaner architecture though. Previously `state` mixed input values and labelWidth. Now the concerns are clearly separated
I suspect that there is a bug related to hooks usage. Need to investigate
Details of bundle changes.Comparing: 5f55f49...6b79818
|
Member
oliviertassinari
approved these changes
Feb 22, 2019
oliviertassinari
added
good first issue
Member
|
The table of content doesn't seem to be working correctly after this change. Select an item. The active item won't change when scrolling. I'm looking into it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
For reviewers: Commits are atomic. Review should be done on a per-commit basis.
Closes #14628
Summary
react-hooksinitially reported ✖ 23 problems (9 errors, 14 warnings)4e4ad7f
next vs. PR
Nothing was working at all. Reason were stale closures.
ab8cf71
next vs. PR
Leaky demo:
0. Use hooks version
Reason were stale closures.
d3bdef7
Linter appeasement in it's current state but the same pattern cause other bugs. Would not consider false positive personally.
c576408
Clear false positive because hooks have a naming convention. Public interface stays the same.
190f8fc
Statement about server performance is misleading. No false positive for me personally because it might be abused.
0f36d32
Caught redundant bailout which was only useful in alpha.
c47fd88
Probably the most complicated diff. No test failed so either we have untested behavior or this is actually a perf improvement (reduces number of window.matchMedia calls to 1 instead of n).
1072aaa
False positive. We need to test this behavior.
b7b897b
Might be a bug in the plugin. I had another instance where it seemed to recognize invariants from the outer scope.
a6737b6
next vs PR
Similar issues to other progress demos
d6fa9d2
Cleaner code
b157068
next vs PR
Fix was a little bit more complicated. SImilar bug to other progress demos
c35ec2a
Not a bug in our docs but if used in other apps can lead to bugs.
49f3eb0
Looks like it fixed #14628 /cc @mbrookes
ffe2a62
Potential bug depending on usage.
6b79818
Needs investigation. False positive for now.For reviewers: Commits are atomic. Review should be done on a per-commit basis.
Closes #14628
Summary
react-hooksinitially reported ✖ 23 problems (9 errors, 14 warnings)4e4ad7f
next vs. PR
Nothing was working at all. Reason were stale closures.
ab8cf71
next vs. PR
Leaky demo:
0. Use hooks version
Reason were stale closures.
d3bdef7
Linter appeasement in it's current state but the same pattern cause other bugs. Would not consider false positive personally.
c576408
Clear false positive because hooks have a naming convention. Public interface stays the same.
190f8fc
Statement about server performance is misleading. No false positive for me personally because it might be abused.
0f36d32
Caught redundant bailout which was only useful in alpha.
c47fd88
Probably the most complicated diff. No test failed so either we have untested behavior or this is actually a perf improvement (reduces number of window.matchMedia calls to 1 instead of n).
1072aaa
False positive. We need to test this behavior.
b7b897b
Might be a bug in the plugin. I had another instance where it seemed to recognize invariants from the outer scope.
a6737b6
next vs PR
Similar issues to other progress demos
d6fa9d2
Cleaner code
b157068
next vs PR
Fix was a little bit more complicated. SImilar bug to other progress demos
c35ec2a
Not a bug in our docs but if used in other apps can lead to bugs.
49f3eb0
Looks like it fixed #14628 /cc @mbrookes
ffe2a62
Potential bug depending on usage.
6b79818
Needs investigation. False positive for now.