Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Galex ESLint config #1726

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Replace Galex ESLint config #1726

wants to merge 1 commit into from

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Jan 10, 2025

Galex is no longer maintained, unfortunately, notably because of the transition to ESLint 9's flat config format. Being dependent on it was starting to cause issues, notably with TypeScript.

Having not found a suitable replacement, I've decided to write my own config, in the hope that we can later use it in other projects at the ESRF.

  • The new config relies on ESLint v9 and the new flat config format — basically one big array of config objects that looks like this:
    [
      {
        name: 'h5web/defaults/rules/<plugin>',
        plugins: { 'my-plugin': myPlugin },
        rules: {
          'my-plugin/my-rule': 'error',
        }
      },
      // ...
    }
  • The set-up remains otherwise the same: one base config file at the root of the monorepo imported by an ESLint config entrypoint (now called eslint.config.js instead of .eslintrc.cjs) inside each project + at the root.
  • Like Galex, the new config detects the dependencies actually installed inside each project (React, TypeScript, etc.) and enables only the relevant plugins and rules.
  • Here is what's happening in terms of plugins:
    • eslint-plugin-tailwindcss and @next/eslint-plugin-next are dropped, obviously
    • eslint-plugin-etc is dropped as it doesn't support ESLint 9 and is not actively maintained
    • eslint-plugin-sonarjs is dropped because most of the rules are either very opinionated/stylistic or duplicate core ESLint rules and rules from other plugins (like the new regexp plugin below)
    • eslint-config-prettier is dropped because I prefer to be careful not to enable stylistic rules that may conflict with Prettier myself (this is made a lot easier by the fact that stylistic rules are no longer part of the core ESLint package)
    • eslint-plugin-jest and related plugins are replaced with eslint-plugin-vitest
    • eslint-plugin-regexp is added to properly lint regular expressions
    • @stylistic/eslint-plugin-js is added for one specific stylistic rule not enforced by Prettier: spaced-comment
  • The new config supports linting JS projects (i.e. without TypeScript) but it does make the assumption that all .js files use ESM (i.e. that package.json has "type": "module").
  • I decided to not mix and match rules from different plugins within each flat config object. Instead, for each plugin, I always have one config object that enables the plugin on as many files as possible and with as many rules as possible, and then additional config objects that disable some of the plugin's rules on specific files (e.g. on TS files, on test files, etc.)

As you'll see with the many changes in the source code, the new config is a little stricter. I'll try to explain what's going on a bit more with some comments.

.eslintignore Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with a config object in each project's eslint.config.js file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a typical entrypoint for a project: it includes detection of dependencies, creation of the base config, and configuration of files to ignore.

apps/demo/src/DemoApp.tsx Show resolved Hide resolved
apps/demo/src/DemoApp.tsx Show resolved Hide resolved
apps/demo/src/vite-env.d.ts Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
} = { ...DEFAULT_OPTS, ...opts };

return tseslint.config(
_.compact([
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lodash's compact utility allows passing withTypeScript && { ... } into the array instead of ...(withTypeScript ? [{ ... }] : []).

'error',
{ checkForEach: true }, // enforce no return in `forEach`
],
// 'arrow-body-style': 'off', // use of curly braces in arrow functions should first be dictated by readabillity
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I declare every single rule explicitly so there are no surprises. I keep those that are disabled commented to not pollute the display when inspecting the config with pnpm --filter @h5web/shared lint:eslint --inspect-config or the like.

withTypeScript && {
/**
* TypeScript plugin: https://typescript-eslint.io/rules/
* Use both `strictTypeChecked` and stylisticTypeChecked` configs, and then tweak.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typescript-eslint is the only plugin for which I use the built-in configs and then tweak the rules (instead of declaring every single rule). There are just too many of them and most are enabled anyway.

@axelboc axelboc requested a review from loichuder January 13, 2025 08:56
const matches = regex.exec(base);

if (!matches) {
throw new Error(`Unrecognized base ${base}`);
}

const [, sign, sizeStr, h5tOrderStr] = matches;
const size = Number.parseInt(sizeStr, 10);
const size = Number.parseInt(sizeStr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this also enforced by a ESLint rule ?

Copy link
Contributor Author

@axelboc axelboc Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it was https://github.com/ljosberinn/eslint-config-galex/blob/1792b61043d9f8e5c08be924f6a999318cfc2cf1/src/plugins/eslint-core.ts#L1176

I've been enforcing this myself thinking it was still relevant. Well, there's still the case that, without a radix specified, the 0x prefix parses the string as hexadecimal. For a user-provided input (e.g. the domain bounds in the domain widget), this might not be what we're after... but then maybe it is (or at least doesn't matter).

if (scaleType !== ScaleType.Log) {
return formatTick;
}

// If available size allows for all log ticks to be rendered without overlap, use default formatter
const [min, max] = domain[0] > 0 ? domain : [-domain[1], -[domain[0]]];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it. Weird it wasn't picked up by TS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants