Additional lint rules to consider

[ryanio]

After working in the monorepo extensively, I compiled some ESLint rules that should help improve dev ex, code quality and style.

These are nothing urgent or breaking and can be picked up over time. (These rules should be added to the top-level config/eslint.js to automatically be included in all packages.)

• strict-boolean-expressions Monorepo: eslint strict boolean expressions #2030
• require-await Extracted to Review and enable require-await rule for eslint #2156 due to its size
• import-sort-order chore: set up eslint-plugin-simple-import-sort and eslint-plugin-import #2086
• object-shorthand chore: enable object-shorthand rule for eslint #2131
• array-foreach chore: enable github/array-foreach rule for eslint #2130
• no-sparse-arrays included via eslint:recommended

[holgerd77]

Cool, thanks for the submission!

Just to clarify: is there anything here which would have (in whatever twists) some "breaking" implications? (otherwise we could take this rather a bit slower in doubt, lot's of other stuff to do/finish)

[ryanio]

nothing breaking, also nothing super urgent, just some suggestions to improve code quality/style, can be done over time 👍

[gabrocheleau]

+1 for strict-boolean-expressions, would have caught an issue that I just spent quite a bit of time troubleshooting :)

[ryanio]

Here's some of my preferred config, hardhat's eslintrc rules are a great resource.

    "@typescript-eslint/consistent-type-imports": "error",
    "import/order": [
      "error",
      {
        alphabetize: {
          order: "asc",
        },
        groups: [
          "object",
          ["builtin", "external"],
          "parent",
          "sibling",
          "index",
          "type",
        ],
        "newlines-between": "always",
      },
    ],
    "sort-imports": ["error", { ignoreDeclarationSort: true }],
    '@typescript-eslint/strict-boolean-expressions': [
      'error',
      {
        allowString: false,
        allowNumber: false,
        allowNullableObject: false,
        allowAny: false,
      },
    ],

[ryanio]

another good one: @typescript-eslint/prefer-nullish-coalescing

[faustbrian]

@ryanio updated your post with links to PRs that enable some of the rules

[faustbrian]

Enabling the @typescript-eslint/strict-boolean-expressions rule with a strict config is quite a headache in the current state of the repository because of the isTruthy and isFalsy functions and the mixed use of null and undefined rather than sticking with undefined since null is generally regarded as a mistake.

[holgerd77]

Enabling the @typescript-eslint/strict-boolean-expressions rule with a strict config is quite a headache in the current state of the repository because of the isTruthy and isFalsy functions and the mixed use of null and undefined rather than sticking with undefined since null is generally regarded as a mistake.

So guess then we would want to get rid again of the isTruthy and isFalsy functions again in a first step (which we would want to do anyhow)?

For undefined/null can you clarify in what circumstances should this be regarded as a mistake? Likely not for all cases where we use null? Maybe/optimally you can give two examples, one with a "false" use case and one with a fitting one?

[faustbrian]

What I mean by mistake is that generally null and undefined mean the same thing and people mostly argue semantics that undefined means uninitialised and null means currently unavailable but both of those are the same thing in terms of code, an empty state. It's basically the whole null-pointer billion-dollar mistake reasoning but also consistency because you don't want to introduce multiple values that have the same meaning.

This sindresorhus/meta#7 is a good read and lists quite a few reasons why you should ditch null in your code. TypeScript itself also doesn't use null and denotes everything undefined with undefined and the Microsoft style-guide also tells developers to use undefined. Also a fun snippet to show how null can trip you up

❯ node
Welcome to Node.js v16.16.0.
Type ".help" for more information.
> typeof null
'object'
> typeof undefined
'undefined'

[faustbrian]

@ryanio I tried to enable require-await but it has tons of false positives like the below code where this.get will return a Promise but because the call isn't awaited it doesn't recognise it correctly. The code is perfectly fine as-is so the rule wouldn't add any benefit.

  async numberToHash(blockNumber: bigint): Promise<Buffer> {
    if (blockNumber < BigInt(0)) {
      throw new NotFoundError(blockNumber)
    }

    return this.get(DBTarget.NumberToHash, { blockNumber })
  }

What do you want to do with that rule? Leave it for now or enable it and disable the false-positives with eslint-disable-next-line? That's a lot disable.

[faustbrian]

@holgerd77 no-sparse-arrays doesn't result in any changes yet and I remember you said we should only add rules that result in an actual change. Does that still stand?

[faustbrian]

@typescript-eslint/strict-boolean-expressions would be the biggest remaining rule, after that only smaller ones. Opened a separate issue for that due to the complexity of the task.

[holgerd77]

@holgerd77 no-sparse-arrays doesn't result in any changes yet and I remember you said we should only add rules that result in an actual change. Does that still stand?

If we don't inflate I guess there is nothing to be said to add selected and reasoned additional ones, so I would be ok with that.

[faustbrian]

no-sparse-arrays is actually already included via the eslint:recommended config that is being extended.

[faustbrian]

@holgerd77 I think we can close this. I enabled all requested rules and extracted the bigger ones into separate issues I'll tackle.

[acolytec3]

Is there any value to keep this open at this point? We've addressed basically all of the open rules.

[jochem-brouwer]

I think we should close this one, but nevertheless review (new) ESLint rules anyways. (We can discuss on the call)

[holgerd77]

Ok, would agree to both of you, will close.