Enable the new rule react/no-unused-class-component-methods and fix eslint errors #3662
Conversation
julienw
force-pushed
the
add-react-no-unused-class-component-methods
branch
from
36d6b5f
to
1f3c1cb
Compare
julienw
changed the title
| version: '15.0', | ||
| flowVersion: '0.63.1', | ||
| version: '17.0', | ||
| flowVersion: '0.96.0', |
There was a problem hiding this comment.
I'm not exactly sure about the usefulness of these properties, but seeing them outdated always makes me crazy so I took the opportunity to update these. It's supposed to trigger different behaviors in the eslint react plugin, but I'm not so sure.
There was a problem hiding this comment.
I'm not sure if it's because of this, but I started to get 4 additional warnings when I tested this on my local.
profiler/src/test/components/StackChart.test.js
240:3 warning Test has no assertions jest/expect-expect
244:3 warning Test has no assertions jest/expect-expect
profiler/src/test/components/TrackContextMenu.test.js
428:5 warning Test has no assertions jest/expect-expect
540:5 warning Test has no assertions jest/expect-expect
Edit: Well, actually it looks like I'm getting that on the main branch as well. I guess I didn't see them earlier because I didn't do a yarn install properly earlier 🤷♂️ . I guess we can fix this in another PR.
There was a problem hiding this comment.
Yeah, I filed this yesterday about that: jest-community/eslint-plugin-jest#988
I think this is due to the latest upate...
This is unfortunate because they skip it for "todo", see jest-community/eslint-plugin-jest#954 ! Then it may be very easy to fix there...
There was a problem hiding this comment.
Oh, interesting. This is definitely something they should fix. Also, maybe we should change these tests to it.todo as well?
There was a problem hiding this comment.
Maybe it.todo would be more appropriate in this case yeah
There was a problem hiding this comment.
Yeah. Also to be clear, I wasn't talking for this PR, we can fix these in another PR.
Codecov Report
@@ Coverage Diff @@
## main #3662 +/- ##
==========================================
+ Coverage 88.77% 88.79% +0.01%
==========================================
Files 259 259
Lines 21748 21731 -17
Branches 5566 5563 -3
==========================================
- Hits 19307 19296 -11
+ Misses 2262 2257 -5
+ Partials 179 178 -1
Continue to review full report at Codecov.
|
| @@ -152,10 +152,6 @@ export class ProfileDeletePanel extends PureComponent<PanelProps, PanelState> { | |||
| } | |||
| }; | |||
|
|
|||
| onCancelDeletion = () => { | |||
| this.props.onProfileDeleteCanceled(); | |||
There was a problem hiding this comment.
This function is used directly in the render function
There was a problem hiding this comment.
That looks like a nice clean-up, thanks! One important thing before merging this is to not forget to rebase this PR and fix the additional errors before landing. Because I added a new class that includes focus function that's not called directly (in the TrackSearchField.js file). We should also ignore this function, otherwise will get perma errors.
| @@ -152,10 +152,6 @@ export class ProfileDeletePanel extends PureComponent<PanelProps, PanelState> { | |||
| } | |||
| }; | |||
|
|
|||
| onCancelDeletion = () => { | |||
| this.props.onProfileDeleteCanceled(); | |||
| version: '15.0', | ||
| flowVersion: '0.63.1', | ||
| version: '17.0', | ||
| flowVersion: '0.96.0', |
There was a problem hiding this comment.
I'm not sure if it's because of this, but I started to get 4 additional warnings when I tested this on my local.
profiler/src/test/components/StackChart.test.js
240:3 warning Test has no assertions jest/expect-expect
244:3 warning Test has no assertions jest/expect-expect
profiler/src/test/components/TrackContextMenu.test.js
428:5 warning Test has no assertions jest/expect-expect
540:5 warning Test has no assertions jest/expect-expect
Edit: Well, actually it looks like I'm getting that on the main branch as well. I guess I didn't see them earlier because I didn't do a yarn install properly earlier 🤷♂️ . I guess we can fix this in another PR.
julienw
force-pushed
the
add-react-no-unused-class-component-methods
branch
from
1f3c1cb
to
ee276a5
Compare
I tried it and found it was useful to do some code maintenance and clean up actually unused instance and class methods and variables.
I did some manual testing and everything seems to work just fine :-)
deploy preview