Suggested improvements for version 2

[filiptammergard]

Suggested improvements for version 2

Currently extended configs and plugins

• eslint-plugin-jest/recommended
• eslint-config-airbnb
• eslint-config-airbnb/hooks
• @typescript-eslint/eslint-plugin/recommended
• eslint-config-prettier/@typescript-eslint
• eslint-plugin-prettier

Suggested rule changes

We are disabling several rules. Some of them are disabled for good reasons and should remain disabled. Others are good rules that probably should be enabled, but are disabled for legacy reasons, to not have to do too many fixes when the eslint-config-airbnb was extended for example. Some of these rules should be enabled to ensure we're following good practices in our new projects.

A general suggestion is to avoid using warnings. Either error or disabled, to prevent warnings from piling up.

prefer-template

Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb/base.

// disallowed
const str = "Hello, " + name + "!";

// allowed
const str = `Hello, ${name}!`;

@typescript-eslint/no-explicit-any

Currently disabled. Is a warning in @typescript-eslint/eslint-plugin/recommended. Suggestion is to make it an error.

// disallowed
const age: any = "seventeen";

@typescript-eslint/no-non-null-assertion

Currently disabled. Suggestion is to remove disable, which will make it an error as in @typescript-eslint/eslint-plugin/recommended.

interface Foo {
  bar?: string;
}

const foo: Foo = getFoo();

// disallowed
const includesBaz = foo.bar!.includes('baz');

// allowed
const includesBaz = foo.bar?.includes('baz') ?? false;

jsx-a11y/anchor-is-valid

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb.

// disallowed
<a onClick={foo} />
<a href="#" onClick={foo} />
<a href="javascript:void(0)" onClick={foo} />
<a />
<a href={undefined} />
<a href={null} />
<a href="#" />
<a href="javascript:void(0)" />

// allowed
<a href={someValidPath} />
<a href={someValidPath} onClick={foo} />

no-else-return

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb/base.

// disallowed
function foo() {
  if (x) {
    return y;
  } else {     
    return z;
  }
}

// allowed
function foo() {
  if (x) {
    return y;
  }

  return z;
}

no-restricted-globals

Currently disabled. isNaN and isFinite are broken but remain for legacy reasons. Number.isNaN and Number.isFinite should be used instead. Some browser globals commonly cause confusion and are not recommended to use without an explicit window- qualifier. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb/base.

// disallowed
isNaN(x);
isFinite(x);
const a = status;

// allowed
Number.isNaN(x);
Number.isFinite(x);
const a = window.status;

no-shadow

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb/base.

// disallowed
var a = 3;

function b() {
  var a = 10;
}

object-shorthand

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb/base.

// disallowed
const foo = {
  x: x,
  y: function () {},
};

// allowed
const foo = {
  x,
  y() {},
};

prefer-destructuring

Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb/base.

const obj = { 
  firstName: "Filip", 
  lastName: "Tammergård"
};
const arr = [1, 2, 3];

// disallowed
const firstName = obj.firstName;
const lastName = obj.lastName;
const first = arr[0];
const second = arr[1];

// allowed
const { firstName, lastName } = obj;
const [first, second] = arr;

react/no-array-index-key

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb.

// disallowed
things.map((thing, index) => (
  <Hello key={index} />
));

// allowed
things.map((thing) => (
  <Hello key={thing.id} />
));

react/jsx-boolean-value

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb.

// disallowed
<Foo disabled={true} />

// allowed
<Foo disabled />
<Foo disabled={!!bar} />

react/jsx-key

Currently warning. Suggestion is to make it an error.

// disallowed
things.map((thing) => (
  <Hello />
));

// allowed
things.map((thing) => (
  <Hello key={thing.id} />
));

react/jsx-filename-extension

Currently warning and .js, .jsx and .tsx are allowed JSX file extensions. Suggestion is to only allow .tsx, since we're exclusively using TypeScript in our React projects. Make it an error instead of warning.

@typescript-eslint/no-unused-vars

Currently disabled. Suggestion is to either remove it, which will make it a warning as in @typescript-eslint/eslint-plugin/recommended, or to make it an error. If "noUnusedLocals": true in tsconfig.json, unused variables will be a ts warning. But we're using the default "noUnusedLocals": false in most projects. And better to use ESLint for linting than tsconfig IMO.

// disallowed
const a = "foo";

// a never used

consistent-return

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb/base.

// disallowed
if (condition) {
  return true;
} else if (condition2) {
  return;
}

// allowed
if (condition) {
  return true;
} else if (condition2) {
  return false;
}

if (condition) {
  return;
} else if (condition2) {
  return;
}

default-case

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb/base, where either an empty default: or a // no default comment satisfies the rule. Omitting default should be done explicitly to make the developer's intention clear.

// disallowed
switch (a) {
  case 1:
    doSomething();
    break;
}

// allowed
switch (a) {
  case 1:
    doSomething();
    break;

  default:
}

switch (a) {
  case 1:
    doSomething();
    break;

  // no default
}

no-restricted-syntax

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb/base, where for...in, for...of, loop labels and with statements are errors for these reasons.

react/jsx-curly-brace-presence

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb.

// disallowed
<App>{"Hello world"}</App>;
<App prop={"Hello world"} attr={"foo"} />;

// allowed
<App>"Hello world"</App>;
<App prop="Hello world" attr="foo" />;

react/jsx-one-expression-per-line

Currently disabled. Suggestion is to remove disable, which will make it an error as in eslint-config-airbnb.

// disallowed
<App><Hello /><Hello2 /></App>

<App><Hello />
  <Hello2 />
</App>

<App>
  <Hello />
  <Hello2 /></App>

// allowed
<App>
  <Hello />
  <Hello2 />
</App>

react/jsx-wrap-multilines

Currently disabled. Suggestion is to remove disabled, which will make it an error as in eslint-config-airbnb.

// disallowed
const hello = (<div>
  <p>Hello</p>
</div>);

// allowed
const hello = (
  <div>
    <p>Hello</p>
  </div>
);

Suggested architectural improvements

React 17

With React 17, react/react-in-jsx-scope and react/jsx-uses-react should be disabled (but enabled with React 16). Possible solutions:

• Let the rules remain in the plugin and disable them in all React 17 projects. (Bad solution IMO, since all our new projects are going to use React 17).
• Disable the rules in the plugin and enable them in the old projects where we want to upgrade to version 2 but continue using React 16.
• Instead of having a default config, we could split it up in one recommended config (with the rules disabled and one react16 config (with the rules enabled).

Development workflow

Some of these rules impair development workflow, when the app breaks if one variable isn't currently used for example when you're trying something out. Do we want to adopt some kind of workflow where some ESLint errors are OK during development, but pushes are rejected? Other ideas?

Dependencies

• Bump dependencies to latest
• Should prettier be a dependency in a ESLint plugin, or rather a peerDependency? If we want prettier as a dependency, should we also include a default .prettierrc that can be extended/overwritten? Use fixed prettier version.

@typescript-eslint/explicit-function-return-type and @typescript-eslint/no-use-before-define

Currently disabled (and should be IMO). But the rules are not enabled in @typescript-eslint/eslint-plugin/recommended that we're extending, so they can be removed from our plugin without any actual change.

no-undef

Currently disabled. It's an error in eslint-config-airbnb/base but disabled in @typescript-eslint/eslint-plugin/recommended which comes later in the extends array. Therefore, we could remove it from our plugin without any actual change.

import/resolver

Currently, .js, .jsx, .ts and .tsx will be parsed as modules and inspected for exports. Suggestion is to change to .ts and .tsx, since we're exclusively using TypeScript in our React projects.

"jest/globals": true

"jest/globals": true is included through eslint-plugin-jest/recommended and can be removed without any actual change.

[filiptammergard]

Comments and additions:

• react/jsx-filename-extension: Keep as is to keep supporting .js and jsx.
• import/resolver: Keep as is to keep supporting .js and jsx.
• Remove disable of react/jsx-props-no-spreading rule, which will make it an error as in eslint-config-airbnb.
• Enforce import organization.
• Disable React 17 rules, and enable them in React 16 projects.

[ericwenn]

Amazing job compiling this list! Agree with almost every suggestion here, and glad to see many of these rules being enabled again.

Will take a closer look next week!