ENH Loosen rules

[emteknetnz]

Issue silverstripe/silverstripe-admin#1411

Follow up issue silverstripe/silverstripe-admin#1433

There is a huge number of linting issues, over 2,000 on admin alone - https://github.com/silverstripe/silverstripe-admin/actions/runs/3869147541/jobs/6595073564#step:12:4367. Fixing this will be a massive job.

I think it would be a very poor use of time to refactor all of these just for the sake of airbnb's updated style guide, I'm also I'm not sure I particularly like some of the things in their style guide anyway, such as prefer destructuring which makes harder to read code IMO.

It's too much work to do this pre-beta. I think we should just ignore all the new failures for now (though raise warnings for some things) and maybe aim to implement some of these rules, though realistically this will struggle to get priorities. Note that AirBNB's style guide is MUCH more comprehensive than PSR2/PSR12

The only other realistic option is too disable linting in CI, though that's pretty awful. Who knows when we'll turn it back on?

Some examples of rules being loosened for existing code - there are very large numbers of examples for most of these.

silverstripe/admin/client/src/stories/ValueTracker.js
48:41 error Must use destructuring props assignment react/destructuring-assignment

silverstripe/admin/client/src/lib/castStringToElement.js
50:23 error Prop spreading is forbidden react/jsx-props-no-spreading

silverstripe/admin/client/src/components/TreeDropdownField/TreeDropdownField.js
739:7 error Use array destructuring prefer-destructuring

"TODO" section of .eslintrc.js - these are things think we should just ignore for now to get the CMS beta over the line and aim to fix at a later time, there are well over a hundred warnings to fix for all of the following. I don't see anything here as particularly high value.:

silverstripe/admin/client/src/containers/InsertLinkModal/tests/InsertLinkModal-test.js
12:1 error Component should be written as a pure function react/prefer-stateless-function

silverstripe/admin/client/src/state/toasts/ToastsReducer.js
117:24 error Default parameters should be last default-param-last

[emteknetnz]

import/no-extraneous-dependencies is no longer the warning being thrown in most cases - but not all caseds, so need to change it to import/no-unresolved

All of the inline // eslint-disable-next-line import/no-extraneous-dependencies scattered out through the codebase probably were not doing much before, so I've removed them all

[GuySartorelli]

so I've removed them all

As in there are separate PRs for this in silverstripe/admin, etc? Where are those PRs? Or did I misunderstand this statement?
Edit: nvm I see your comment further down saying we shouldn't touch the other modules for now. Agreed.

[emteknetnz]

This didn't used to throw errors, possibly the detection is now better

[emteknetnz]

Little script I wrote to fix a few of the things in silverstripe/admin, though I don't think we should update modules at this point

php -r '
$fs = glob("client/src/{,*/,*/*/,*/*/*/,*/*/*/*/,*/*/*/*/*/}*.js", GLOB_BRACE);
foreach ($fs as $f) {
  $c = file_get_contents($f);
  $c = preg_replace("/\/\* global.+?\n/", "", $c);
  $c = preg_replace("#(?<!Number\.)isNaN\(#", "Number.isNaN(", $c);
  $c = str_replace("// eslint-disable-next-line import/no-extraneous-dependencies\n", "", $c);
  file_put_contents($f, $c);
  foreach (["jest", "tinymce", "editorIdentifier"] as $s) {
    if ($s == "editorIdentifier") {
      if (!str_contains($c, "$s")) continue;
    } else {
      if (!str_contains($c, "$s.")) continue;
    }
    if (preg_match("#/\* global .*?$s#", $s)) continue;
    if (!str_contains($c, "/* global")) {
      $c = "/* global */\n$c";
      file_put_contents($f, $c);
    }
    $c = str_replace("/* global", "/* global $s", $c);
    file_put_contents($f, $c);
  }
}
'

To automatically file issues similar to phpcbf:
node_modules/.bin/eslint --fix client/src

[emteknetnz]

200 character line limit is required to support things like this - https://github.com/silverstripe/silverstripe-admin/blob/1/client/src/lib/dependency-injection/tests/ApolloGraphqlScaffoldingContainer-test.js#L164

Presumably there previously wasn't a line limit

[GuySartorelli]

Arguably things like that shouldn't exist, and should be refactored into something we can actually read and interpret. Something for us to consider when revisiting these rules.

[GuySartorelli]

Looks okay with the caveat that we will be revisiting this post-beta to make more intentional decisions on these rule changes.