From 584d0bafb07310d37be94b823dbf1a409cd041f1 Mon Sep 17 00:00:00 2001 From: Victor Diez Date: Mon, 6 Nov 2023 11:32:57 +0100 Subject: [PATCH 1/3] Add rule S6788 --- .../sonar/javascript/checks/CheckList.java | 1 + .../javascript/checks/NoFindDomNodeCheck.java | 36 +++++++++++++ .../javascript/rules/javascript/S6788.html | 51 +++++++++++++++++++ .../javascript/rules/javascript/S6788.json | 28 ++++++++++ .../rules/javascript/Sonar_way_profile.json | 1 + 5 files changed, 117 insertions(+) create mode 100644 sonar-plugin/javascript-checks/src/main/java/org/sonar/javascript/checks/NoFindDomNodeCheck.java create mode 100644 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6788.html create mode 100644 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6788.json diff --git a/sonar-plugin/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java b/sonar-plugin/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java index db31f1e386..2a4d7f4f51 100644 --- a/sonar-plugin/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java +++ b/sonar-plugin/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java @@ -265,6 +265,7 @@ public static List> getAllChecks() { NoExtendNativeCheck.class, NoExtraBindCheck.class, NoExtraBooleanCastCheck.class, + NoFindDomNodeCheck.class, NoForInArrayCheck.class, NoHardcodedIpCheck.class, NoHookSetterInBodyCheck.class, diff --git a/sonar-plugin/javascript-checks/src/main/java/org/sonar/javascript/checks/NoFindDomNodeCheck.java b/sonar-plugin/javascript-checks/src/main/java/org/sonar/javascript/checks/NoFindDomNodeCheck.java new file mode 100644 index 0000000000..f95dfe6d6c --- /dev/null +++ b/sonar-plugin/javascript-checks/src/main/java/org/sonar/javascript/checks/NoFindDomNodeCheck.java @@ -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"; + } +} diff --git a/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6788.html b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6788.html new file mode 100644 index 0000000000..536efa95ce --- /dev/null +++ b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6788.html @@ -0,0 +1,51 @@ +

Why is this an issue?

+

In React, findDOMNode is used to get the browser DOM node given a component instance. However, using findDOMNode 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 findDOMNode. There are also other caveats when using findDOMNode.

+
+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 <input defaultValue="Hello" />
+  }
+}
+
+

Instead, one should get the component’s own DOM node from a ref. Pass your ref as the ref attribute to the JSX tag for which you want +to get the DOM node. This tells React to put this <input>’s DOM node into inputRef.current.

+

Use createRef to manage a specific DOM node. In modern React without class components, the equivalent code would call +useRef instead.

+
+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 (
+      <input ref={this.inputRef} defaultValue="Hello" />
+    );
+  }
+}
+
+

Resources

+

Documentation

+ + diff --git a/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6788.json b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6788.json new file mode 100644 index 0000000000..2250c873d9 --- /dev/null +++ b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6788.json @@ -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" + ] +} diff --git a/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json index 38d39b0a02..e8ab3af123 100644 --- a/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json +++ b/sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json @@ -292,6 +292,7 @@ "S6772", "S6774", "S6775", + "S6788", "S6793", "S6807", "S6811", From 4dec9289a891bab14511fd12ca397704f56f06b1 Mon Sep 17 00:00:00 2001 From: Victor Diez Date: Mon, 6 Nov 2023 14:59:29 +0100 Subject: [PATCH 2/3] Decorate rule message Add ruling results --- .../jsts/console/typescript-S6788.json | 20 ++++++++ .../jsts/desktop/typescript-S6788.json | 6 +++ .../jsts/sonar-web/javascript-S6788.json | 19 +++++++ .../sonar/javascript/it/JsTsRulingTest.java | 48 +---------------- packages/jsts/src/rules/S6788/decorator.ts | 34 +++++++++++++ packages/jsts/src/rules/S6788/index.ts | 23 +++++++++ packages/jsts/src/rules/S6788/unit.test.ts | 51 +++++++++++++++++++ packages/jsts/src/rules/index.ts | 2 + 8 files changed, 156 insertions(+), 47 deletions(-) create mode 100644 its/ruling/src/test/expected/jsts/console/typescript-S6788.json create mode 100644 its/ruling/src/test/expected/jsts/desktop/typescript-S6788.json create mode 100644 its/ruling/src/test/expected/jsts/sonar-web/javascript-S6788.json create mode 100644 packages/jsts/src/rules/S6788/decorator.ts create mode 100644 packages/jsts/src/rules/S6788/index.ts create mode 100644 packages/jsts/src/rules/S6788/unit.test.ts diff --git a/its/ruling/src/test/expected/jsts/console/typescript-S6788.json b/its/ruling/src/test/expected/jsts/console/typescript-S6788.json new file mode 100644 index 0000000000..52cffbd365 --- /dev/null +++ b/its/ruling/src/test/expected/jsts/console/typescript-S6788.json @@ -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 +] +} diff --git a/its/ruling/src/test/expected/jsts/desktop/typescript-S6788.json b/its/ruling/src/test/expected/jsts/desktop/typescript-S6788.json new file mode 100644 index 0000000000..8da2854019 --- /dev/null +++ b/its/ruling/src/test/expected/jsts/desktop/typescript-S6788.json @@ -0,0 +1,6 @@ +{ +"desktop:app/src/ui/lib/list/list.tsx": [ +1016, +1240 +] +} diff --git a/its/ruling/src/test/expected/jsts/sonar-web/javascript-S6788.json b/its/ruling/src/test/expected/jsts/sonar-web/javascript-S6788.json new file mode 100644 index 0000000000..0894b20eca --- /dev/null +++ b/its/ruling/src/test/expected/jsts/sonar-web/javascript-S6788.json @@ -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 +] +} diff --git a/its/ruling/src/test/java/org/sonar/javascript/it/JsTsRulingTest.java b/its/ruling/src/test/java/org/sonar/javascript/it/JsTsRulingTest.java index a4bd9fdd0f..03c7648680 100644 --- a/its/ruling/src/test/java/org/sonar/javascript/it/JsTsRulingTest.java +++ b/its/ruling/src/test/java/org/sonar/javascript/it/JsTsRulingTest.java @@ -88,55 +88,9 @@ class JsTsRulingTest { public static Stream ruling() { return Stream.of( - jsTsProject("amplify", "external/**", "test"), - jsTsProject("angular.js", "src/ngLocale/**, i18n/**", "test"), - jsTsProject("backbone", "test"), - jsTsProject("es5-shim", "tests"), - jsTsProject("fireact"), - jsTsProject("ace"), - jsTsProject("ecmascript6-today"), - jsTsProject("expressionist.js"), - jsTsProject("Ghost"), - jsTsProject("http"), - jsTsProject("reddit-mobile"), - jsTsProject("redux"), - jsTsProject("router"), - jsTsProject("snoode"), jsTsProject("sonar-web"), - jsTsProject("templating"), - jsTsProject("watchtower.js"), - jsTsProject("jira-clone"), - jsTsProject("jquery", "test"), - jsTsProject("jshint", "tests"), - jsTsProject("jStorage", "tests"), - jsTsProject("knockout", "spec"), - jsTsProject("mootools-core", "Specs"), - jsTsProject("ocanvas", "build/**", ""), - jsTsProject("p5.js", "test"), - jsTsProject("paper.js", "gulp/jsdoc/**, packages/**", "test"), - jsTsProject("prototype", "test"), - jsTsProject("qunit", "test"), - jsTsProject("react-cloud-music"), - jsTsProject("sizzle", "external/**", "test"), - jsTsProject("underscore", "test"), - jsTsProject("ag-grid", "spec"), - jsTsProject("ant-design", "tests"), // todo: many dirs **/__tests__ jsTsProject("console"), // todo: many dirs **/__tests__ - jsTsProject("courselit", ".yarn/**", ""), - jsTsProject("desktop", "app/test"), - jsTsProject("eigen"), // todo - jsTsProject("fireface"), - jsTsProject("ionic2-auth"), - jsTsProject("Joust"), // todo: files **/*.spec.ts - jsTsProject("moose"), - jsTsProject("postgraphql"), // todo: many dirs **/__tests__ - jsTsProject("prettier-vscode"), - jsTsProject("rxjs", "spec"), - jsTsProject("searchkit"), // todo - jsTsProject("TypeScript", "src/harness/unittests"), - jsTsProject("vuetify"), - jsTsProject("yaml", "../sources/yaml", "", ""), - jsTsProject("file-for-rules", "../sources/jsts/custom", "", "tests") + jsTsProject("desktop", "app/test") ); } diff --git a/packages/jsts/src/rules/S6788/decorator.ts b/packages/jsts/src/rules/S6788/decorator.ts new file mode 100644 index 0000000000..f8e7445d26 --- /dev/null +++ b/packages/jsts/src/rules/S6788/decorator.ts @@ -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."; + return interceptReport(rule, (context, reportDescriptor) => { + context.report({ + ...reportDescriptor, + }); + }); +} diff --git a/packages/jsts/src/rules/S6788/index.ts b/packages/jsts/src/rules/S6788/index.ts new file mode 100644 index 0000000000..b38bb0534b --- /dev/null +++ b/packages/jsts/src/rules/S6788/index.ts @@ -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']); diff --git a/packages/jsts/src/rules/S6788/unit.test.ts b/packages/jsts/src/rules/S6788/unit.test.ts new file mode 100644 index 0000000000..6505a32bf9 --- /dev/null +++ b/packages/jsts/src/rules/S6788/unit.test.ts @@ -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
+ } +} + `, + errors: [ + { + message: + "Do not use findDOMNode. It doesn't work with function components and is deprecated in StrictMode.", + }, + ], + }, + ], +}); diff --git a/packages/jsts/src/rules/index.ts b/packages/jsts/src/rules/index.ts index 6aa1a24977..fcab226e49 100644 --- a/packages/jsts/src/rules/index.ts +++ b/packages/jsts/src/rules/index.ts @@ -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 @@ -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; From 4626fe91b0b806c0cc59cfdd5f9228437b5b62d0 Mon Sep 17 00:00:00 2001 From: Victor Diez Date: Mon, 6 Nov 2023 15:17:44 +0100 Subject: [PATCH 3/3] revert changes on JsTsRulingTest.java --- .../sonar/javascript/it/JsTsRulingTest.java | 48 ++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/its/ruling/src/test/java/org/sonar/javascript/it/JsTsRulingTest.java b/its/ruling/src/test/java/org/sonar/javascript/it/JsTsRulingTest.java index 03c7648680..a4bd9fdd0f 100644 --- a/its/ruling/src/test/java/org/sonar/javascript/it/JsTsRulingTest.java +++ b/its/ruling/src/test/java/org/sonar/javascript/it/JsTsRulingTest.java @@ -88,9 +88,55 @@ class JsTsRulingTest { public static Stream ruling() { return Stream.of( + jsTsProject("amplify", "external/**", "test"), + jsTsProject("angular.js", "src/ngLocale/**, i18n/**", "test"), + jsTsProject("backbone", "test"), + jsTsProject("es5-shim", "tests"), + jsTsProject("fireact"), + jsTsProject("ace"), + jsTsProject("ecmascript6-today"), + jsTsProject("expressionist.js"), + jsTsProject("Ghost"), + jsTsProject("http"), + jsTsProject("reddit-mobile"), + jsTsProject("redux"), + jsTsProject("router"), + jsTsProject("snoode"), jsTsProject("sonar-web"), + jsTsProject("templating"), + jsTsProject("watchtower.js"), + jsTsProject("jira-clone"), + jsTsProject("jquery", "test"), + jsTsProject("jshint", "tests"), + jsTsProject("jStorage", "tests"), + jsTsProject("knockout", "spec"), + jsTsProject("mootools-core", "Specs"), + jsTsProject("ocanvas", "build/**", ""), + jsTsProject("p5.js", "test"), + jsTsProject("paper.js", "gulp/jsdoc/**, packages/**", "test"), + jsTsProject("prototype", "test"), + jsTsProject("qunit", "test"), + jsTsProject("react-cloud-music"), + jsTsProject("sizzle", "external/**", "test"), + jsTsProject("underscore", "test"), + jsTsProject("ag-grid", "spec"), + jsTsProject("ant-design", "tests"), // todo: many dirs **/__tests__ jsTsProject("console"), // todo: many dirs **/__tests__ - jsTsProject("desktop", "app/test") + jsTsProject("courselit", ".yarn/**", ""), + jsTsProject("desktop", "app/test"), + jsTsProject("eigen"), // todo + jsTsProject("fireface"), + jsTsProject("ionic2-auth"), + jsTsProject("Joust"), // todo: files **/*.spec.ts + jsTsProject("moose"), + jsTsProject("postgraphql"), // todo: many dirs **/__tests__ + jsTsProject("prettier-vscode"), + jsTsProject("rxjs", "spec"), + jsTsProject("searchkit"), // todo + jsTsProject("TypeScript", "src/harness/unittests"), + jsTsProject("vuetify"), + jsTsProject("yaml", "../sources/yaml", "", ""), + jsTsProject("file-for-rules", "../sources/jsts/custom", "", "tests") ); }