Eslint - Removed legacyCode rules

- Fix any issues in files being ignored - Fixed linting errors where appropriate - Include some rules being ignored in the src/components folder - Closes #8731
2022-12-17 15:32:55 +01:00 · 2022-12-17 15:32:55 +01:00 · 554311752b
commit 554311752b
--- a/.eslintrc.js
+++ b/.eslintrc.js
@ -1,12 +1,3 @@
-// Rules which have been enforced in configuration upgrades and flag issues in existing code.
-// We need to consider whether to disable those rules permanently, or fix the issues.
-const legacyCode = {
-  'default-param-last': 'off',
-  'no-continue': 'off',
-  'no-else-return': 'off',
-  'no-restricted-syntax': 'off',
-};
-
 module.exports = {
  extends: [
    '@wagtail/eslint-config-wagtail',
@ -63,26 +54,15 @@ module.exports = {
    'import/resolver': { node: { extensions: ['.js', '.ts', '.tsx'] } },
  },
  overrides: [
-    // Legacy Code - remove from `files` when adopting desired rules in new code progressively
-    {
-      files: [
-        'client/src/entrypoints/admin/core.js',
-        'client/src/entrypoints/admin/page-editor.js',
-        'client/src/entrypoints/admin/telepath/widgets.js',
-        'client/src/entrypoints/contrib/typed_table_block/typed_table_block.js',
-        'client/src/utils/version.js',
-      ],
-      rules: legacyCode,
-    },
    // Rules that we are ignoring currently due to legacy code in React components only
    {
      files: ['client/src/components/**'],
      rules: {
-        ...legacyCode,
        'jsx-a11y/click-events-have-key-events': 'off',
        'jsx-a11y/interactive-supports-focus': 'off',
        'jsx-a11y/no-noninteractive-element-interactions': 'off',
        'jsx-a11y/role-supports-aria-props': 'off',
+        'no-restricted-syntax': 'off',
        'react-hooks/exhaustive-deps': 'off',
        'react-hooks/rules-of-hooks': 'off',
        'react/button-has-type': 'off',
--- a/client/src/components/ChooserWidget/index.js
+++ b/client/src/components/ChooserWidget/index.js
@ -53,9 +53,8 @@ export class Chooser {
        state[this.editUrlStateKey] = this.editLink.getAttribute('href');
      }
      return state;
-    } else {
-      return null;
    }
+    return null;
  }

  getState() {
--- a/client/src/components/CommentApp/utils/layout.ts
+++ b/client/src/components/CommentApp/utils/layout.ts
@ -188,6 +188,7 @@ export class LayoutController {
                  pinnedCommentPosition - previousBlock.pinnedCommentPosition;
              }

+              // eslint-disable-next-line no-continue
              continue;
            }
          }
--- a/client/src/components/Draftail/CommentableEditor/CommentableEditor.tsx
+++ b/client/src/components/Draftail/CommentableEditor/CommentableEditor.tsx
@ -417,7 +417,8 @@ export function findLeastCommonCommentId(block: ContentBlock, offset: number) {
  const styleCount = styles.count();
  if (styleCount === 0) {
    return null;
-  } else if (styleCount > 1) {
+  }
+  if (styleCount > 1) {
    // We're dealing with overlapping comments.
    // Find the least frequently occurring style and use that - this isn't foolproof, but in
    // most cases should ensure that all comments have at least one clickable section. This
@ -890,7 +891,8 @@ function CommentableEditor({
                return {
                  backgroundColor: background,
                };
-              } else if (numStyles > 1) {
+              }
+              if (numStyles > 1) {
                // Otherwise if we're in a region with overlapping comments, use a different colour than usual
                // to indicate that
                background = overlappingHighlight;
--- a/client/src/components/PageExplorer/reducers/explorer.ts
+++ b/client/src/components/PageExplorer/reducers/explorer.ts
@ -41,7 +41,7 @@ export type Action =
 * - Whether the explorer is open or not.
 */
 export default function explorer(
-  prevState = defaultState,
+  prevState = defaultState /* eslint-disable-line default-param-last */,
  action: Action,
 ): State {
  switch (action.type) {
--- a/client/src/components/PageExplorer/reducers/nodes.ts
+++ b/client/src/components/PageExplorer/reducers/nodes.ts
@ -131,7 +131,10 @@ export type Action =
 /**
 * A single page node in the explorer.
 */
-const node = (state = defaultPageState, action: Action): PageState => {
+const node = (
+  state = defaultPageState /* eslint-disable-line default-param-last */,
+  action: Action,
+): PageState => {
  switch (action.type) {
    case GET_PAGE_SUCCESS:
      return { ...state, ...action.payload.data, isError: false };
@ -194,7 +197,10 @@ const defaultState: State = {};
 /**
 * Contains all of the page nodes in one object.
 */
-export default function nodes(state = defaultState, action: Action) {
+export default function nodes(
+  state = defaultState /* eslint-disable-line default-param-last */,
+  action: Action,
+) {
  switch (action.type) {
    case OPEN_EXPLORER: {
      return { ...state, [action.payload.id]: { ...defaultPageState } };
--- a/client/src/components/Sidebar/Sidebar.tsx
+++ b/client/src/components/Sidebar/Sidebar.tsx
@ -63,16 +63,15 @@ export const Sidebar: React.FunctionComponent<SidebarProps> = ({
      if (checkWindowSizeIsMobile()) {
        setIsMobile(true);
        return null;
-      } else {
-        setIsMobile(false);
-
-        // Close the menu and animate out as this state is not used in desktop
-        setVisibleOnMobile(false);
-        // wait for animation to finish then hide menu from screen readers as well.
-        return setTimeout(() => {
-          setClosedOnMobile(true);
-        }, SIDEBAR_TRANSITION_DURATION);
      }
+      setIsMobile(false);
+
+      // Close the menu and animate out as this state is not used in desktop
+      setVisibleOnMobile(false);
+      // wait for animation to finish then hide menu from screen readers as well.
+      return setTimeout(() => {
+        setClosedOnMobile(true);
+      }, SIDEBAR_TRANSITION_DURATION);
    }

    window.addEventListener('resize', handleResize);
--- a/client/src/entrypoints/admin/core.js
+++ b/client/src/entrypoints/admin/core.js
@ -99,7 +99,8 @@ function enableDirtyFormCheck(formSelector, options) {
  const isFormDirty = () => {
    if (alwaysDirty) {
      return true;
-    } else if (!initialData) {
+    }
+    if (!initialData) {
      return false;
    }

@ -116,7 +117,8 @@ function enableDirtyFormCheck(formSelector, options) {
      const oldValue = initialData.get(key);
      if (newValue === oldValue) {
        return false;
-      } else if (Array.isArray(newValue) && Array.isArray(oldValue)) {
+      }
+      if (Array.isArray(newValue) && Array.isArray(oldValue)) {
        return (
          newValue.length !== oldValue.length ||
          newValue.some((value, index) => value !== oldValue[index])
@ -155,29 +157,22 @@ function enableDirtyFormCheck(formSelector, options) {

      $form.on('change keyup', updateDirtyCheck).trigger('change');

-      const validInputNodeInList = (nodeList) => {
-        for (const node of nodeList) {
-          if (
-            node.nodeType === node.ELEMENT_NODE &&
-            ['INPUT', 'TEXTAREA', 'SELECT'].includes(node.tagName)
-          ) {
-            return true;
-          }
-        }
-        return false;
-      };
+      const isValidInputNode = (node) =>
+        node.nodeType === node.ELEMENT_NODE &&
+        ['INPUT', 'TEXTAREA', 'SELECT'].includes(node.tagName);

      const observer = new MutationObserver((mutationList) => {
-        for (const mutation of mutationList) {
-          if (
-            validInputNodeInList(mutation.addedNodes) ||
-            validInputNodeInList(mutation.removedNodes)
-          ) {
-            updateDirtyCheck();
-            return;
-          }
+        const hasMutationWithValidInputNode = mutationList.some(
+          (mutation) =>
+            mutation.addedNodes.some(isValidInputNode) ||
+            mutation.removedNodes.some(isValidInputNode),
+        );
+
+        if (hasMutationWithValidInputNode) {
+          updateDirtyCheck();
        }
      });
+
      observer.observe($form[0], {
        childList: true,
        attributes: false,
--- a/client/src/entrypoints/admin/page-editor.js
+++ b/client/src/entrypoints/admin/page-editor.js
@ -133,10 +133,11 @@ window.updateFooterSaveWarning = (formDirty, commentsDirty) => {
  }
  clearTimeout(updateFooterTextTimeout);
  const updateWarnings = () => {
-    for (const warning of warnings) {
+    warnings.forEach((warning) => {
      const visible = typeVisibility[warning.dataset.unsavedType];
+      // eslint-disable-next-line no-param-reassign
      warning.hidden = !visible;
-    }
+    });
  };
  if (hiding) {
    // If hiding, we want to keep the text as-is before it disappears
--- a/client/src/entrypoints/admin/telepath/widgets.js
+++ b/client/src/entrypoints/admin/telepath/widgets.js
@ -295,6 +295,7 @@ class BoundDraftailWidget {
    if (!value || !value.blocks) return '';

    let result = '';
+    // eslint-disable-next-line no-restricted-syntax
    for (const block of value.blocks) {
      if (block.text) {
        result += result ? ' ' + block.text : block.text;
--- a/client/src/entrypoints/contrib/typed_table_block/typed_table_block.js
+++ b/client/src/entrypoints/contrib/typed_table_block/typed_table_block.js
@ -1,3 +1,4 @@
+/* eslint-disable no-restricted-syntax */
 /* global $ */

 import { escapeHtml as h } from '../../../utils/text';
--- a/client/src/utils/version.js
+++ b/client/src/utils/version.js
@ -61,7 +61,8 @@ class VersionNumber {
      (that.preReleaseStep === 'b' || that.preReleaseStep === 'rc')
    ) {
      return true;
-    } else if (this.preReleaseStep === 'b' && that.preReleaseStep === 'rc') {
+    }
+    if (this.preReleaseStep === 'b' && that.preReleaseStep === 'rc') {
      return true;
    }
    return false;
@ -73,9 +74,11 @@ class VersionNumber {
  howMuchBehind(that) {
    if (this.major < that.major) {
      return VersionDeltaType.MAJOR;
-    } else if (this.major === that.major && this.minor < that.minor) {
+    }
+    if (this.major === that.major && this.minor < that.minor) {
      return VersionDeltaType.MINOR;
-    } else if (
+    }
+    if (
      this.major === that.major &&
      this.minor === that.minor &&
      !this.isPreRelease() &&
@ -83,16 +86,19 @@ class VersionNumber {
      this.patch < that.patch
    ) {
      return VersionDeltaType.PATCH;
-    } else if (
+    }
+    if (
      this.major === that.major &&
      this.minor === that.minor &&
      this.isPreRelease()
    ) {
      if (!that.isPreRelease()) {
        return VersionDeltaType.MINOR;
-      } else if (this.isPreReleaseStepBehind(that)) {
+      }
+      if (this.isPreReleaseStepBehind(that)) {
        return VersionDeltaType.PRE_RELEASE_STEP;
-      } else if (
+      }
+      if (
        this.preReleaseStep === that.preReleaseStep &&
        this.preReleaseVersion < that.preReleaseVersion
      ) {