Update ESLint configuration

[tomast1337]

Summary
This PR consolidates significant improvements to the project's linting infrastructure, code quality enforcement, and test reliability. The changes focus on standardizing code style, updating ESLint configuration to use modern TypeScript ESLint, and resolving workspace dependency issues that were causing test failures.

---

LLM's description of the rules:

General Configuration

• Ignored Files: The configuration begins by explicitly ignoring common directories that should not be linted, such as node_modules, build output folders (dist, build, .next), and configuration files.
• Base Rules: It extends the recommended defaults from ESLint (js.configs.recommended) and TypeScript-ESLint (tseslint.configs.recommended) to provide a strong foundation of general best practices.
• Environment: The global settings are configured for a modern Node.js environment (globals.node) and support ES2021 features.
• Plugins: It utilizes several key plugins:
  • eslint-plugin-import: For managing and ordering import statements.
  • @stylistic/eslint-plugin: For enforcing a consistent code style and format.
  • eslint-plugin-react: For enforcing React best practices and rules for JSX.

---

Rule Summary with Examples

Here is a breakdown of what each specific rule enforces.

1. Code Quality & Best Practices

• no-console: Warns if console.log() or other console methods are used.
  • Incorrect: console.log('Debugging data');
  • Correct: Using a dedicated logger or removing the log statement.
• max-len: Sets a maximum line length of 1024 characters but ignores comments, URLs, and strings to remain practical.
• @typescript-eslint/no-explicit-any: Warns against using the any type to encourage stronger type safety.
  • Incorrect: let user: any = response.data;
  • Correct: let user: UserProfile = response.data;
• @typescript-eslint/no-require-imports: Disallows require() in favor of ES module import statements.
  • Incorrect: const path = require('path');
  • Correct: import path from 'path';
• @typescript-eslint/ban-ts-comment: Warns against using suppression comments like // @ts-ignore.
• @typescript-eslint/no-unused-vars: Finds variables that are declared but never used. It is configured to ignore variables prefixed with an underscore (_).
  • Incorrect: const unusedValue = getData();
  • Correct: const _unusedValue = getData(); or removing it.

2. Import Rules (eslint-plugin-import)

• import/order: Enforces a strict order for imports, grouping them logically (built-in, external, internal, etc.). It is configured to treat paths starting with @/ as internal.
  • Incorrect:

    import MyComponent from '@/components/MyComponent';
    import fs from 'fs';
    import React from 'react';

  • Correct:

    import fs from 'fs';
    
    import React from 'react';
    
    import MyComponent from '@/components/MyComponent';

• import/newline-after-import: Requires a blank line after the import block.
• import/no-duplicates: Combines imports from the same module into a single line.
  • Incorrect: import { A } from 'module'; import { B } from 'module';
  • Correct: import { A, B } from 'module';

3. Stylistic & Formatting Rules (@stylistic/eslint-plugin)

• @stylistic/indent: Enforces 4-space indentation for most files. (This is overridden to 2 spaces for JSX/TSX files).
• @stylistic/space-infix-ops: Requires spaces around operators.
  • Incorrect: const sum=a+b;
  • Correct: const sum = a + b;
• @stylistic/keyword-spacing: Requires spaces around keywords like if, else, for.
  • Incorrect: if(condition){...}
  • Correct: if (condition) {...}
• @stylistic/arrow-spacing: Requires spaces around the arrow in arrow functions.
  • Incorrect: const add=(a, b)=>a + b;
  • Correct: const add = (a, b) => a + b;
• @stylistic/space-before-blocks: Requires a space before a block's opening brace.
  • Incorrect: function myFunc(){...}
  • Correct: function myFunc() {...}
• @stylistic/object-curly-spacing: Requires spaces inside object braces.
  • Incorrect: const user = {name: 'John'};
  • Correct: const user = { name: 'John' };
• @stylistic/comma-spacing: Requires a space after a comma, not before.
  • Incorrect: const list = [1 , 2 , 3];
  • Correct: const list = [1, 2, 3];
• @stylistic/comma-dangle: Disallows trailing commas.
  • Incorrect: const user = { name: 'John', };
  • Correct: const user = { name: 'John' };
• @stylistic/key-spacing: Enforces consistent spacing around the colon in object properties.
  • Incorrect: const user = { name : 'John' };
  • Correct: const user = { name: 'John' };

4. React-Specific Rules (eslint-plugin-react)

• react.configs.recommended.rules: This applies a large set of recommended best-practice rules for React, including:
  • react/jsx-key: Warns if you forget to add a key prop to elements in a list.
  • react/jsx-no-duplicate-props: Prevents defining the same prop twice on a component.
  • react/jsx-no-target-blank: Ensures rel="noreferrer" is present on links with target="_blank" for security.
• react/react-in-jsx-scope: 'off': This rule is turned off because modern versions of React (17+) do not require you to import React from 'react' in every file that uses JSX.
• react/no-unknown-property: Prevents you from using non-standard HTML attributes on DOM elements, which helps catch typos. This rule has been configured to specifically allow the following custom attributes:
  • custom-prop
  • cmdk-input-wrapper
  • cmdk-group-heading
  • Example: <div cmdk-input-wrapper="">...</div> will now be considered valid.

5. File-Specific Overrides

• For .jsx and .tsx files: The global indentation rule is overridden to enforce 2 spaces instead of 4, which is a common convention in the React community.
  • Correct JSX Indentation:

    function MyComponent() {
      return (
        <div>
          <p>Hello, World!</p>
        </div>
      );
    }

[Bentroen]

Thank you for opening the pull request!

I've given considerable thought to the proposed formatting changes, but I'm not particularly fond of applying them to the codebase.

The crux of the issue is that formatting styles are highly personal. It's entirely fine, in a personal project or one maintained by a closed team, to tweak formatting options so the code looks better to those involved in maintaining it.
But ultimately, in a public project, it's crucial to consider that others will be reading the source code. In a few years' time, it may not be us maintaining this codebase. Therefore, we have the duty to make decisions that will make the most sense in the long run. And if these rules are the default today, there's likely a good reason why — and that's a compelling reason to keep following them.

📝Formatting vs. linting

The ESLint team has decided to stop maintaining all style-related rules in version 8, having deferred this responsibility to the community. They have a great reason to do that, as maintaining these formatting rules was taking a great chunk of their time.

There's a strong case for not using ESLint to format code — linters and formatters are different tools, and tying up both operations makes formatting as slow as linting. Lately, I have noticed that source.fixAll on save in NBW's codebase is starting to take over a minute on my machine, a problem that scales with the number of linting rules and lines of code in the project.

Prettier is already integrated with the project and largely takes care of standardizing code style, thanks to its opinionated nature.

Before migrating to ESLint 8, we were using whitespace rules to insert line breaks. The result is that this forced line breaks in a few awkward places where it wouldn't be necessary to add them. Nowadays, I would probably reconsider this addition — in many cases, the programmer knows best how to format their code, and these rules end up being too black and white.

Proponents of @eslint/stylistic argue that, while Prettier is amazing as an out-of-the-box solution, it falls short of providing customization options where it does make sense to use them.

However, I don't believe we have reached the point where this level of control would achieve a significant improvement in our workflow, to make it worth introducing such rules.

The consequence of leaning heavily into stylistic rules is that it suddenly opens up a can of worms — there are 95 available rules in @eslint/stylistic that could potentially be tweaked to improve the project.

The time spent considering these rules and tweaking them so the code looks the best on every possible instance of these constructs adds up to a vast portion of our development time. And, although it's a 'set-and-forget' type of task, there is the possibility of some of these rules being discontinued in the future, or even ESLint changing enough that the behavior/implementation of these rules would change.

Each of these rules can be considered a separate dependency, and, like every dependency, it must be maintained.
Each rule has a configuration, and the format for specifying them often changes across subsequent versions. Each rule we choose to introduce adds a recurring time investment cost, which adds up over time. The more we stray from a simple configuration, the more complex the project becomes, and the greater the maintenance burden we'll incur.

In this case, it is much more beneficial to strive for simplicity than to have absolute customizability. Formatting rules are, for the most part, minor details. Sensible defaults go a long way, and, to a great extent, can be considered 'good enough'. I'd rather go with a widely used standard, but 'live with' limited customizability, than to spend a lot of time reasoning about these rules, and how they affect our codebase (decisions which often involve very subjective criteria; time that could be better spent developing actual features).

An acceptable alternative would be to implement an opinionated set of rules, such as antfu/eslint-config. This defers the maintenance burden over these rules to someone else rather than us — unless, of course, we decide to customize it all ourselves.
If we go with this route, I've just learned it's possible to disable the editor's squiggly lines for some of the linting rules (see here and here), so it'd act more as an auto-formatter fixing your code silently rather than screaming errors at you :)

Lastly, we had agreed upon these defaults much earlier on the project, back when there wasn't nearly as much code around. Changing these now, amidst all the refactorings we are doing on the codebase, generates a lot of noise that's difficult to reason about, with little benefit to the project. I believe the most beneficial approach would be to stick with what we have already picked at the start — otherwise, what's preventing us from repeatedly changing the formatting style again down the line?

---

On the specific formatting changes you suggested:

• Line width 1024: I can see the argument for using a larger line width — ultrawide or even 4K monitors are commonplace today —, but we have to work under the assumption that not every developer who could be involved has access to one of these. There's a reason why line lengths are kept around 100 on large open-source projects, and recommended as such by many style guides and formatters (such as black and prettier itself). 1024 allows for practically unlimited line length — it acts almost like not enforcing line lengths at all. Since lines typically seldom go past ~120 characters anyway, it's reasonable to keep the limit around that. The increase in vertical length should be marginal, and horizontal space will be kept within sensible bounds.

• Colon alignment: I can appreciate how better it looks visually. But this puts more emphasis on the colons than on the content they hold. Besides, it spawns an artificial need for reformatting the entire block if a longer key is introduced, inflating diffs unnecessarily. Finally, it makes the horizontal space problem worse overall, pushing a lot of text to the right.

• Two vs. four indentation spaces: I believe that selectively picking the indent size per file type creates some inconsistencies across the project. It so happened before on the project that we changed a .ts file's extension to .tsx so we could use JSX's features. A move like this would cause the whole indentation in the file to change.
  Two spaces across the entire codebase is quite a sensible decision for a JS/TS project. I'd prefer that we go all the way through with four spaces instead of selectively picking it based on the file type; however, for JSX and other deeply nested constructs, this would impair readability. Additionally, it also contributes to the horizontal space issue.

• All other stylistic changes: they're already taken care of by Prettier, so it's not necessary to specify them.

All in all, I believe the best course of action is just to pick the default values and go with them.

---

📍 Proposal

I suggest we:

• remove all stylistic rules from ESLint;
• tweak Prettier's printWidth to 120 and let it handle the codebase's formatting;
• add back the following rule: 'lines-between-class-members': ['warn', 'always', { exceptAfterSingleLine: true }]
• turn on formatting on save for VS Code;
• set pre-commit hooks to run formatting + linting on every commit.

No rule conflicts, no maintenance burden, no disagreements, sensible defaults all the way. ✅

---

🟢 Case in point: Open Collective's frontend codebase

Available at: https://github.com/opencollective/opencollective-frontend

• Minimal Prettier config (printWidth 120)
• Heavy use of ESLint, but only for linting, not styling (other than enforcing line breaks here and there)
• Pre-commit hook running lint-staged

This goes a long way! 🚀

If a project as large as Open Collective has no reason to touch these rules, I don't think we have either ;)

---

There's still the issue that JSON and other configuration files have no associated configuration. As such, simply saving many of these files in my end reformats them to a different line break/indentation style (I'd have to figure out why).

I agree with all the other linting changes! Though they did generate lots of warnings — it could be interesting to go through them at some point and fix them.

[tomast1337]

@Bentroen
I agree with your proposal and will update the PR to remove the stylistic rules and rely on Prettier.

Regarding the pre-commit hook, I have some reservations. They introduce significant friction to the development process. They slow down commits, which discourages making small, frequent "checkpoints," and can feel like a roadblock when you want to save your progress. They also add configuration bloat (like Husky), can be inconsistent across different machines, and are easily bypassed.

Wouldn't it open to relying on format on save instead? It achieves the same goal by ensuring code is clean as we write. We could then use a CI check on the PR as a final backstop to catch anything that slips through, as it is.

[Bentroen]

Thank you for making the changes!

I'll do one final linting commit later as the final step before merging the PR.

As for the pre-commit hook, I agree with the points you mentioned. We'd mostly be relying on format on save anyway; by following our recommended practice, other contributors would also rarely commit unformatted code. I suggested it more as a 'final resort' to catch potential slips early, rather than at the end of a PR — where many formatting changes could be necessary to get everything in order.

I've thought about going with a middle-ground solution: in parallel with a CI action (which would only point out what's wrong instead of fixing it), we could set up a pre-commit hook only on specific branches, to make sure all code committed there adheres to the formatting and linting rules. It seems to be possible to configure pre-commit only on specific branches, see here.
Though I'm fine with keeping this pull request as-is and postponing this change to a future PR, as it requires some further thought. ;)