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.

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

Create rule S6788: Disallow usage of findDOMNode (react/no-find-dom-node) #4359

Merged
merged 3 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions its/ruling/src/test/expected/jsts/console/typescript-S6788.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"console:src/components/Help/Alert.tsx": [
28
],
"console:src/components/Help/Help.tsx": [
28
],
"console:src/components/PopupWrapper/PopupWrapper.tsx": [
56
],
"console:src/views/ProjectRootView/AddModelPopup.tsx": [
163
],
"console:src/views/ProjectRootView/EditModelPopup.tsx": [
186
],
"console:src/views/ProjectSettingsView/ProjectSettingsView.tsx": [
216
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"desktop:app/src/ui/lib/list/list.tsx": [
1016,
1240
]
}
19 changes: 19 additions & 0 deletions its/ruling/src/test/expected/jsts/sonar-web/javascript-S6788.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"sonar-web:src/main/js/components/mixins/resize-mixin.js": [
19
],
"sonar-web:src/main/js/components/mixins/tooltips-mixin.js": [
16
],
"sonar-web:src/main/js/main/nav/component/component-nav.js": [
37
],
"sonar-web:tests/apps/background-tasks-test.js": [
68
],
"sonar-web:tests/apps/system-test.js": [
15,
73,
89
]
}
34 changes: 34 additions & 0 deletions packages/jsts/src/rules/S6788/decorator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* SonarQube JavaScript Plugin
* Copyright (C) 2011-2023 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
// https://sonarsource.github.io/rspec/#/rspec/S6788/javascript

import { Rule } from 'eslint';
import { interceptReport } from '../helpers';

// Rewording issue message reported by the core implementation.
export function decorate(rule: Rule.RuleModule): Rule.RuleModule {
rule.meta!.messages!['noFindDOMNode'] =
"Do not use findDOMNode. It doesn't work with function components and is deprecated in StrictMode.";
Copy link
Contributor

Choose a reason for hiding this comment

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

I am never sure, but is "function component", "functional component", or both can be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both can be used. at least in the docs they use both

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return interceptReport(rule, (context, reportDescriptor) => {
context.report({
...reportDescriptor,
});
});
}
23 changes: 23 additions & 0 deletions packages/jsts/src/rules/S6788/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* SonarQube JavaScript Plugin
* Copyright (C) 2011-2023 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
import { rules } from 'eslint-plugin-react';
import { decorate } from './decorator';

export const rule = decorate(rules['no-find-dom-node']);
51 changes: 51 additions & 0 deletions packages/jsts/src/rules/S6788/unit.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* SonarQube JavaScript Plugin
* Copyright (C) 2011-2023 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
import { rule } from './';
import { TypeScriptRuleTester } from '../tools';

const ruleTester = new TypeScriptRuleTester();

ruleTester.run(`Decorated rule should reword the issue message`, rule, {
valid: [
{
code: `type Foo = () => number;`,
},
],
invalid: [
{
code: `
class MyComponent extends Component {
componentDidMount() {
findDOMNode(this).scrollIntoView();
}
render() {
return <div />
}
}
`,
errors: [
{
message:
"Do not use findDOMNode. It doesn't work with function components and is deprecated in StrictMode.",
},
],
},
],
});
2 changes: 2 additions & 0 deletions packages/jsts/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ import { rule as S3415 } from './S3415'; // inverted-assertion-arguments
import { rule as S6477 } from './S6477'; // jsx-key
import { rule as S6481 } from './S6481'; // jsx-no-constructed-context-values
import { rule as S6749 } from './S6749'; // jsx-no-useless-fragment
import { rule as S6788 } from './S6788'; // no-find-dom-node
import { rule as S1439 } from './S1439'; // label-position
import { rule as S5148 } from './S5148'; // link-with-target-blank
import { rule as S4622 } from './S4622'; // max-union-size
Expand Down Expand Up @@ -388,6 +389,7 @@ rules['inverted-assertion-arguments'] = S3415;
rules['jsx-key'] = S6477;
rules['jsx-no-constructed-context-values'] = S6481;
rules['jsx-no-useless-fragment'] = S6749;
rules['no-find-dom-node'] = S6788;
rules['label-position'] = S1439;
rules['link-with-target-blank'] = S5148;
rules['max-union-size'] = S4622;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ public static List<Class<? extends JavaScriptCheck>> getAllChecks() {
NoExtendNativeCheck.class,
NoExtraBindCheck.class,
NoExtraBooleanCastCheck.class,
NoFindDomNodeCheck.class,
NoForInArrayCheck.class,
NoHardcodedIpCheck.class,
NoHookSetterInBodyCheck.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* SonarQube JavaScript Plugin
* Copyright (C) 2011-2023 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.sonar.javascript.checks;

import org.sonar.check.Rule;
import org.sonar.plugins.javascript.api.EslintBasedCheck;
import org.sonar.plugins.javascript.api.JavaScriptRule;
import org.sonar.plugins.javascript.api.TypeScriptRule;

@TypeScriptRule
@JavaScriptRule
@Rule(key = "S6788")
public class NoFindDomNodeCheck implements EslintBasedCheck {

@Override
public String eslintKey() {
return "no-find-dom-node";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<h2>Why is this an issue?</h2>
<p>In React, <code>findDOMNode</code> is used to get the browser DOM node given a component instance. However, using <code>findDOMNode</code> is
fragile because the connection between the JSX node and the code manipulating the corresponding DOM node is not explicit. For example, changing the
external structure of returned JSX will affect the return value of <code>findDOMNode</code>. There are also other <a
href="https://react.dev/reference/react-dom/findDOMNode#caveats">caveats</a> when using <code>findDOMNode</code>.</p>
<pre data-diff-id="1" data-diff-type="noncompliant">
import { Component } from 'react';
import { findDOMNode } from 'react-dom';

class AutoselectingInput extends Component {
componentDidMount() {
const input = findDOMNode(this); // Noncompliant: findDOMNode is deprecated
input.select();
}

render() {
return &lt;input defaultValue="Hello" /&gt;
}
}
</pre>
<p>Instead, one should get the component’s own DOM node from a ref. Pass your ref as the <code>ref</code> attribute to the JSX tag for which you want
to get the DOM node. This tells React to put this <code>&lt;input&gt;</code>’s DOM node into <code>inputRef.current</code>.</p>
<p>Use <code>createRef</code> to manage a specific DOM node. In modern React without class components, the equivalent code would call
<code>useRef</code> instead.</p>
<pre data-diff-id="1" data-diff-type="compliant">
import { createRef, Component } from 'react';

class AutoselectingInput extends Component {
inputRef = createRef(null);

componentDidMount() {
const input = this.inputRef.current; // Always points to the input element
input.select();
}

render() {
return (
&lt;input ref={this.inputRef} defaultValue="Hello" /&gt;
);
}
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> React Documentation - <a href="https://react.dev/reference/react-dom/findDOMNode"><code>findDOMNode</code></a> </li>
<li> React Documentation - <a href="https://react.dev/reference/react/createRef"><code>createRef</code></a> </li>
<li> React Documentation - <a href="https://react.dev/reference/react/useRef"><code>useRef</code></a> </li>
<li> React Documentation - <a href="https://react.dev/learn/manipulating-the-dom-with-refs">Manipulating the DOM with Refs</a> </li>
</ul>

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"title": "React\u0027s \"findDOMNode\" should not be used",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": [
"react"
],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-6788",
"sqKey": "S6788",
"scope": "All",
"quickfix": "unknown",
"code": {
"impacts": {
"MAINTAINABILITY": "MEDIUM",
"RELIABILITY": "LOW"
},
"attribute": "CONVENTIONAL"
},
"compatibleLanguages": [
"JAVASCRIPT",
"TYPESCRIPT"
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@
"S6772",
"S6774",
"S6775",
"S6788",
"S6793",
"S6807",
"S6811",
Expand Down