Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable the new rule react/no-unused-class-component-methods and fix eslint errors #3662

Merged
merged 2 commits into from Nov 24, 2021
Merged

Enable the new rule react/no-unused-class-component-methods and fix eslint errors #3662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee276a5
Enable the new rule react/no-unused-class-component-methods and fix e…
julienw Nov 22, 2021
34ad55b
Merge branch 'main' into add-react-no-unused-class-component-methods
julienw Nov 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions .eslintrc.js
View file
Open in desktop
Expand Up @@ -36,6 +36,7 @@ module.exports = {
'react/no-did-update-set-state': 'error',
'react/no-will-update-set-state': 'error',
'react/no-redundant-should-component-update': 'error',
'react/no-unused-class-component-methods': 'error',
'react/no-this-in-sfc': 'error',
'react/no-typos': 'error',
// Flow provides enough coverage over the prop types, and there can be errors
Expand Down Expand Up @@ -128,8 +129,8 @@ module.exports = {
settings: {
react: {
pragma: 'React',
version: '15.0',
flowVersion: '0.63.1',
version: '17.0',
flowVersion: '0.96.0',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting. This is definitely something they should fix. Also, maybe we should change these tests to it.todo as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it.todo would be more appropriate in this case yeah

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Also to be clear, I wasn't talking for this PR, we can fix these in another PR.

},
'import/resolver': {
alias: {
Expand Down
5 changes: 0 additions & 5 deletions src/components/app/MenuButtons/Permalink.js
View file
Open in desktop
Expand Up @@ -26,11 +26,7 @@ type State = {|
|};

export class MenuButtonsPermalink extends React.PureComponent<Props, State> {
_permalinkButton: ButtonWithPanel | null;
_permalinkTextField: HTMLInputElement | null;
_takePermalinkButtonRef = (elem: ButtonWithPanel | null) => {
this._permalinkButton = elem;
};
_takePermalinkTextFieldRef = (elem: HTMLInputElement | null) => {
this._permalinkTextField = elem;
};
Expand Down Expand Up @@ -74,7 +70,6 @@ export class MenuButtonsPermalink extends React.PureComponent<Props, State> {
<Localized id="MenuButtons--permalink--button" attrs={{ label: true }}>
<ButtonWithPanel
buttonClassName="menuButtonsButton menuButtonsButton-hasIcon menuButtonsPermalinkButtonButton"
ref={this._takePermalinkButtonRef}
label="Permalink"
initialOpen={this.props.isNewlyPublished}
onPanelOpen={this._shortenUrlAndFocusTextFieldOnCompletion}
Expand Down
15 changes: 0 additions & 15 deletions src/components/app/MenuButtons/Publish.js
View file
Open in desktop
Expand Up @@ -201,21 +201,6 @@ class MenuButtonsPublishImpl extends React.PureComponent<PublishProps> {
);
}

_closePanelAfterUpload = () => {
const { resetUploadState } = this.props;
// Only reset it after the panel animation disappears.
setTimeout(resetUploadState, 300);

const { body } = document;
if (body) {
// This is a hack to close the arrow panel. See the following issue on
// moving this to the Redux state.
//
// https://github.com/firefox-devtools/profiler/issues/1888
body.dispatchEvent(new MouseEvent('mousedown'));
}
};

_renderUploadPanel() {
const {
uploadProgress,
Expand Down
4 changes: 0 additions & 4 deletions src/components/app/ProfileDeleteButton.js
View file
Open in desktop
Expand Up @@ -152,10 +152,6 @@ export class ProfileDeletePanel extends PureComponent<PanelProps, PanelState> {
}
};

onCancelDeletion = () => {
this.props.onProfileDeleteCanceled();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is used directly in the render function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense.

};

preventClick(e: SyntheticMouseEvent<>) {
e.preventDefault();
}
Expand Down
2 changes: 2 additions & 0 deletions src/components/flame-graph/FlameGraph.js
View file
Open in desktop
Expand Up @@ -122,6 +122,8 @@ class FlameGraphImpl extends React.PureComponent<Props> {
this._viewport = viewport;
};

/* This method is called from MaybeFlameGraph. */
/* eslint-disable-next-line react/no-unused-class-component-methods */
focus = () => {
if (this._viewport) {
this._viewport.focus();
Expand Down
2 changes: 0 additions & 2 deletions src/components/js-tracer/index.js
View file
Open in desktop
Expand Up @@ -40,8 +40,6 @@ type StateProps = {|
type Props = ConnectedProps<{||}, StateProps, DispatchProps>;

class JsTracerImpl extends React.PureComponent<Props> {
_rafGeneration: number = 0;

render() {
const { profile, jsTracerTable, showJsTracerSummary, threadsKey } =
this.props;
Expand Down
5 changes: 5 additions & 0 deletions src/components/shared/ButtonWithPanel/ArrowPanel.js
View file
Open in desktop
Expand Up @@ -35,6 +35,10 @@ export class ArrowPanel extends React.PureComponent<Props, State> {
openGeneration: 0,
};

/* These 2 methods are called from other components.
/* See https://github.com/firefox-devtools/profiler/issues/1888 and
* https://github.com/firefox-devtools/profiler/issues/1641 */
/* eslint-disable-next-line react/no-unused-class-component-methods */
open() {
if (this.state.open) {
return;
Expand All @@ -43,6 +47,7 @@ export class ArrowPanel extends React.PureComponent<Props, State> {
this.setState({ open: true });
}

/* eslint-disable-next-line react/no-unused-class-component-methods */
close() {
this.setState((state) => {
if (!state.open) {
Expand Down
2 changes: 2 additions & 0 deletions src/components/shared/TrackSearchField.js
View file
Open in desktop
Expand Up @@ -21,6 +21,8 @@ export class TrackSearchField extends React.PureComponent<Props> {
this.props.onSearch(e.currentTarget.value);
};

/* This is called from TrackContextMenu directly */
/* eslint-disable-next-line react/no-unused-class-component-methods */
focus = () => {
if (this.searchFieldInput.current) {
this.searchFieldInput.current.focus();
Expand Down
4 changes: 4 additions & 0 deletions src/components/shared/TreeView.js
View file
Open in desktop
Expand Up @@ -429,6 +429,8 @@ export class TreeView<DisplayData: Object> extends React.PureComponent<
{ limit: 1 }
);

/* This method is used by users of this component. */
/* eslint-disable-next-line react/no-unused-class-component-methods */
scrollSelectionIntoView() {
const { selectedNodeId, tree } = this.props;
if (this._list && selectedNodeId !== null) {
Expand Down Expand Up @@ -703,6 +705,8 @@ export class TreeView<DisplayData: Object> extends React.PureComponent<
}
};

/* This method is used by users of this component. */
/* eslint-disable-next-line react/no-unused-class-component-methods */
focus() {
if (this._list) {
this._list.focus();
Expand Down
6 changes: 6 additions & 0 deletions src/components/shared/VirtualList.js
View file
Open in desktop
Expand Up @@ -160,6 +160,8 @@ class VirtualListInner<Item> extends React.PureComponent<
this._container = element;
};

/* This method is used by users of this component. */
/* eslint-disable-next-line react/no-unused-class-component-methods */
getBoundingClientRect() {
if (this._container) {
return this._container.getBoundingClientRect();
Expand Down Expand Up @@ -339,6 +341,8 @@ export class VirtualList<Item> extends React.PureComponent<
return { visibleRangeStart, visibleRangeEnd };
}

/* This method is used by users of this component. */
/* eslint-disable-next-line react/no-unused-class-component-methods */
scrollItemIntoView(itemIndex: number, offsetX: CssPixels) {
const container = this._container.current;
if (!container) {
Expand Down Expand Up @@ -370,6 +374,8 @@ export class VirtualList<Item> extends React.PureComponent<
}
}

/* This method is used by users of this component. */
/* eslint-disable-next-line react/no-unused-class-component-methods */
focus() {
const container = this._container.current;
if (container) {
Expand Down
3 changes: 0 additions & 3 deletions src/components/stack-chart/Canvas.js
View file
Open in desktop
Expand Up @@ -88,9 +88,6 @@ const FONT_SIZE = 10;
const BORDER_OPACITY = 0.4;

class StackChartCanvasImpl extends React.PureComponent<Props> {
_leftMarginGradient: null | CanvasGradient = null;
_rightMarginGradient: null | CanvasGradient = null;

componentDidUpdate(prevProps) {
// We want to scroll the selection into view when this component
// is mounted, but using componentDidMount won't work here as the
Expand Down
3 changes: 0 additions & 3 deletions src/components/timeline/EmptyThreadIndicator.js
View file
Open in desktop
Expand Up @@ -38,9 +38,6 @@ type Props = {|
* thread was shut down.
*/
class EmptyThreadIndicatorImpl extends PureComponent<Props> {
_canvas: HTMLCanvasElement | null;
_requestedAnimationFrame: boolean | null;

render() {
const style = getIndicatorPositions(this.props);
return (
Expand Down
4 changes: 3 additions & 1 deletion src/components/timeline/OriginsTimeline.js
View file
Open in desktop
Expand Up @@ -72,7 +72,9 @@ class OriginsTimelineView extends React.PureComponent<Props, State> {
/**
* This method collects the initially selected track's HTMLElement. This allows the timeline
* to scroll the initially selected track into view once the page is loaded.
*/
* It's not used currently but it will be used when we implement this
* component better. */
/* eslint-disable-next-line react/no-unused-class-component-methods */
setInitialSelected = (el: InitialSelectedTrackReference) => {
this.setState({ initialSelected: el });
};
Expand Down
2 changes: 0 additions & 2 deletions src/components/tooltip/Tooltip.js
View file
Open in desktop
Expand Up @@ -29,8 +29,6 @@ type PositionFromMouse = 'before-mouse' | 'after-mouse';
type TooltipPosition = PositionFromMouse | 'window-edge';

export class Tooltip extends React.PureComponent<Props> {
_isMounted: boolean = false;
_isLayoutScheduled: boolean = false;
_interiorElementRef: {| current: HTMLDivElement | null |} = React.createRef();

// This keeps the previous tooltip positioning relatively to the mouse cursor.
Expand Down