pixiebrix/eslint-config-pixiebrix

Custom eslint config, to be used with Prettier

eslint-config-pixiebrix

ESLint shareable config for PixieBrix

Install

$ npm install --save-dev eslint-config-pixiebrix

Usage

Add some ESLint config to your package.json:

{
	"name": "my-pixiebrix-project",
	"eslintConfig": {
		"extends": "pixiebrix"
	}
}

Or to .eslintrc:

{
	"extends": "pixiebrix"
}

npm publishing

Collaborators can publish a new version of what's on main via "workflow_dispatch" under Actions » Publish

eslint-config-pixiebrix's Issues

Restore automated publishing

The branch protections recently enabled don't allow the automated publishing workflow to push some changes back to the repo:

eslint-config-pixiebrix/.github/workflows/npm-publish.yml

Line 38 in b7beed8

- run: git push --follow-tags

This means that the version in the repo remains out of date (on the first successful publishing) and then future attempts will fail completely.

There might be a solution, but it needs setup/exploration:

https://github.com/orgs/community/discussions/13836#discussioncomment-5341603

Add eslint-plugin-no-only-tests

Background

During Cypress development, we use it.only() to run a single test. See Cypress docs here: https://docs.cypress.io/guides/core-concepts/writing-and-organizing-tests#Excluding-and-Including-Tests

I occasionally forget to remove .only() after developing a single test. Would be nice if eslint catches this mistake. Looks like there's an eslint plugin for it: https://github.com/levibuzolic/eslint-plugin-no-only-tests

Ensure `import()`ed dependencies are always dynamically imported

If you first import('react') and then import "react" in the same bundle, Webpack will not create a secondary chunk, it will just inline it.

In a large application, it's not easy to keep track of all the import decisions, so one optimization (deferred loading via import()) can be undone and left unnoticed forever.

We can now enforce that some dependencies are always dynamically imported:

• ban the static imports of a set of dependencies (i.e. always import canvas-confetti via import())
• remind developers to update this set of dependencies every time the linter encounters a new import("new-dependency")

Both of these are enforceable via no-restricted-syntax

Related

• First mentioned: pixiebrix/pixiebrix-extension#2286 (comment)
• Solution: import-js/eslint-plugin-import#2326 (comment)

Replace `release-with-changelog` step in publish action

Background

The NPM publish action is emitted the following warning:

The set-output command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

The warning is coming from the fregante/release-with-changelog@v3 step.

Per the project repo, the action is deprecated and recommends using the gh release create command instead.

Restrict imports of classnames to cx

Motivation

• We want to enforce our convention of prefering import cx from 'classnames' over import classnames from 'classnames'

Acceptance Criteria

• Find an existing linting rule that would allow us to enforce this
  OR
• Create our own custom linting rule

Notes

• ESLint Plugin Import has a draft PR to provide a rule that would support this
• The draft PR can be found here

Replace `import/no-restricted-paths` with hand-rolled version?

$ TIMING=1 nr lint
Rule                                            | Time (ms) | Relative
:-----------------------------------------------|----------:|--------:
import/no-restricted-paths                      | 71530.176 |    56.0%
@typescript-eslint/promise-function-async       | 10734.587 |     8.4%
@typescript-eslint/no-unsafe-argument           |  5984.000 |     4.7%
@typescript-eslint/no-floating-promises         |  3182.511 |     2.5%
import/no-self-import                           |  2680.865 |     2.1%
@typescript-eslint/no-confusing-void-expression |  1603.084 |     1.3%
@typescript-eslint/no-redeclare                 |  1254.564 |     1.0%
unicorn/no-thenable                             |   898.581 |     0.7%
react/no-direct-mutation-state                  |   799.667 |     0.6%
unicorn/prevent-abbreviations                   |   799.234 |     0.6%

I believe that's an excessive amount of time to what's essentially string matching. Given it doubles the lint time (locally and on CI), we could probably think about fixing it, also because it's an important rule for the extension.

Update "unicorn/prevent-abbreviations" to allow "mod" abbreviation

Background

• As part of the Naming changes, we need to allowlist "mod" as an abbreviation
• Relevant code can be found here

Enable "No Explicit Any" Linting Rule

• https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-explicit-any.md

Forbid import of WritableDraft with `no-restricted-imports`

- import { type WritableDraft } from "immer/dist/internal";
+ import { type Draft } from "immer";

Context: pixiebrix/pixiebrix-extension#6731 (comment)

Some immer types use WritableDraft but that seems to be the wrong type to use, and it's not clear from the errors what the preferred type is.

The rule can do this easily

Implement custom plugins

It's possible that we can enforce some of our practices via small custom plugins. It's something worth looking into

https://backlight.dev/blog/best-practices-w-eslint-part-1

Create local rule to validate isMounted usage in useAsyncEffect

Context

• We use https://www.npmjs.com/package/use-async-effect throughout the Extension
• The callback function has an isMounted function arg
• It is easy to forget to invoke isMounted when working with useAsyncEffect
• We should create a local eslint rule to ensure we remember to invoke isMounted

Enable more React rules

Once all of the rules mentioned on this issue are merged or closed, let's use this issue to migrate those rules from the Extension's eslintrc.js to eslint-config-pixiebrix.

Notes

The config only uses the recommended list, but apparently there are lot more rules:

https://github.com/jsx-eslint/eslint-plugin-react/tree/master#list-of-supported-rules

A very useful one is react/jsx-no-useless-fragment, which I'll enable soon in a PR.

Q: Do you see any other rules that might be useful in that list?

Note: A lot of them are formatting related and that's already taken care by Prettier.

Forbid `expectContext`/`forbidContext` in folders that already enforce it

no-restricted-paths already enforces context and makes the build fail if they're imported by the wrong context.

expectContext/forbidContext calls are redundant. Can we drop them?

Disable background imports in content script files and vice versa

This could export a backgroundContext config to be set on /background/* files in order to exclude direct /options/* and /contentScript/* imports. Same goes for contentScriptContext, etc.

Only */messenger/api imports will be allowed across contexts.

---

Not sure how they can be combined since there can only be one no-restricted-imports rule.

Related:

• pixiebrix/pixiebrix-extension#2050
• https://github.com/pixiebrix/pixiebrix-app/issues/494
• pixiebrix/pixiebrix-extension#669

ESLint Plugin Import - prefer-default-export

https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/prefer-default-export.md

Add `plugin:storybook/recommended`

The Storybook 7 migration guide suggests adding this config

Add no-use-before-define

Implement the no-use-before-define ESLint rule

Disable `jsx-a11y` in test files

Enforce removal of events on `addEventListener`

Every addEventListener must be followed by a removeEventListener or else they'll be left behind and cause hard-to-debug issues:

• pixiebrix/pixiebrix-extension#4258

Enforcing removeEventListener might be difficult, but requiring a signal could be done via types. For example:

addEventListener(
	type: string,
	listener: EventListenerOrEventListenerObject,
-	options?: boolean | AddEventListenerOptions
+	options: AddEventListenerOptions & {signal: AbortSignal}
): void;

Or just via no-restricted-syntax.

---

The most recent issue I found was this listener, left behind between reloads:

https://github.com/pixiebrix/pixiebrix-extension/blob/4878e51b4466cf6d628b5085c6ac964510a924f4/src/contentScript/selectionTooltip/tooltipController.tsx#L281-L299

`@typescript-eslint/naming-convention` configuration

I dropped this rule because it's too complex and incomplete. If we want to enforce some naming conventions with the linter I suggest we use someone else's config and stick to it without exceptions or else we (I) will end up doing this:

Unfortunately the biggest drawback of Sindre Sorhus’ configuration for this rule excludes UPPER_SNAKE_CASE, which you might not like.

From https://github.com/pixiebrix/pixiebrix-app/pull/1246#discussion_r811072628

Related:

• https://github.com/pixiebrix/pixiebrix-app/issues/845
• pixiebrix/pixiebrix-extension#2387
• b12ef09

Enforce the exhaustive deps for our custom hooks

Context

• We use some custom hooks that have deps like useEffect: useAsyncState and useAsyncEffect (which is a library)
• We use an eslint plugin for enforcing exhaustive deps for useEffect and useCallback: https://reactjs.org/docs/hooks-rules.html
• A missing dependency can cause subtle bugs

Implementation Sketch

• Use the advanced configuration to add the hooks to eslint: https://www.npmjs.com/package/eslint-plugin-react-hooks#advanced-configuration

Update eqeqeq eslint rule

• https://eslint.org/docs/latest/rules/eqeqeq
• Set the "null" option to "never" to enforce using loose equality checks for null
  • Loose checks for null will also catch undefined

Implementation Sketch

• Switch from smart to always
• Add the optional { null: "never" } second afgument
• Open/merge PR
• Use the Publish workflow to publish a new patch version of the repo
• Install the latest version in the Extension project
• Run the linter and fix any issues it finds

Replace `import/no-restricted-paths` with `no-restricted-imports`

With a recent update, eslint became unreasonably slow, with lints running at 7 minutes locally. I think this started happening after a ts-eslint or import-plugin update. A good chunk of time seems to be taken by the import/no-restricted-paths rule currently, like 2 whole minutes.

I think the native no-restricted-imports could match the functionality, with the exception that it does not accept "per-directory" options. This can be achieved by using the native eslint overrides, but these overrides cancel out any other config for this rule, unless you manually re-apply it, like:

eslint-config-pixiebrix/index.js

Lines 266 to 278 in 81e6a3e

	"no-restricted-imports": [
	"error",
	{
	// Documentation: https://eslint.org/docs/rules/no-restricted-imports#options
	patterns: [
	// Extend the existing patterns
	...config.rules["no-restricted-imports"][1].patterns,
	{
	group: ["./*"],
	message:
	'Use root-based imports (`import "@/something"`) instead of relative imports.',
	},
	],

Replace `eslint-plugin-eslint-comments` with community version

Unmaintained (2020):

• https://github.com/mysticatea/eslint-plugin-eslint-comments

Semi-official fork:

• https://github.com/eslint-community/eslint-plugin-eslint-comments

Enforce using `ts-extras` instead of Object.entries

pixiebrix/pixiebrix-extension#2052 (comment)

Move to flat config

https://eslint.org/blog/2022/08/new-config-system-part-2/

This would let us reuse rules by just changing names (I presume) instead of manually merging them each time, like:

eslint-config-pixiebrix/index.js

Lines 266 to 278 in 81e6a3e

	"no-restricted-imports": [
	"error",
	{
	// Documentation: https://eslint.org/docs/rules/no-restricted-imports#options
	patterns: [
	// Extend the existing patterns
	...config.rules["no-restricted-imports"][1].patterns,
	{
	group: ["./*"],
	message:
	'Use root-based imports (`import "@/something"`) instead of relative imports.',
	},
	],

This change would make the following change more reasonable:

• #35

eslint.org/docs/latest/use/configure/configuration-files-new

Suggested in #176 (comment)

I'm not sure if this is immediately possible due to our dependencies, but I can look into it if desired.

Move some custom rules from the extension to the shared config

I think someone suggested this before but I can't find it. Maybe it was just me?

I don’t think any of these rules is extension-specific, so it can be shared with other repos

https://github.com/pixiebrix/pixiebrix-extension/blob/main/eslint-local-rules/

    "local-rules/noNullRtkQueryArgs": "error",
    "local-rules/noInvalidDataTestId": "error",
    "local-rules/noExpressionLiterals": "error",
    "local-rules/notBothLabelAndLockableProps": "error",
    "local-rules/preferNullish": "warn",
    "local-rules/preferNullishable": "warn",

Only persistBackgroundData and noCrossBoundaryImports are really specific to the extension I think.

cc @grahamlangford @fungairino

Related

• #186

Implement Testing Library React ESLint Plugin

• Implement https://www.npmjs.com/package/eslint-plugin-jest
  • recommended configuration
• Implement https://www.npmjs.com/package/eslint-plugin-testing-library
  • "plugin:testing-library/react"
• Install to Extension and resolve errors

Automatically enable strict rules on files appearing in `tsconfig.strictNullChecks`

There are some rules that were disabled due to the loose config. We could enable them automatically on the files that appear in the new config.

This might not be practical, depending on how many errors they trigger, but it might be worth a check.

Rules:

https://github.com/pixiebrix/pixiebrix-extension/blob/ecc09cc5ebb3b55791762983be861d0ca786128a/.eslintrc.js#L79-L82

eslint-config-pixiebrix/index.js

Lines 240 to 244 in 0d5501d

	"@typescript-eslint/no-unsafe-assignment": "off",
	"@typescript-eslint/no-unsafe-member-access": "off",
	"@typescript-eslint/no-unsafe-return": "off",
	"@typescript-eslint/no-unsafe-call": "off",
	"@typescript-eslint/prefer-nullish-coalescing": "off", // Requires strictNullChecks

Example usage in the overrides array:

    {
      files: require("./src/tsconfig.strictNullChecks.json").files,
      rules: {
        "@typescript-eslint/no-explicit-any": "error",
      },
    },

Drop `react/jsx-no-target-blank` (noopener, noreferrer requirement)

The rel="noopener" doesn't apply to us, as we had recently found out: All cross-origin links automatically have that rel since 2018-ish and chrome-extension already never appears in the referrer.

I'll be disabling https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-target-blank.md on the project soon.

Use FlatConfig instead of deprecated "eslintrc"

https://eslint.org/docs/latest/use/configure/configuration-files-new

Suggested in #176 (comment)

I'm not sure if this is immediately possible due to our dependencies, but I can look into it if desired.

Enable more `@typescript-eslint/no-unsafe-*` rules

They'll definitely need their own issues. We'll worry about them later on. They're probably good candidates to start off as warnings.

Originally posted by @grahamlangford in #73 (comment)

eslint-config-pixiebrix/index.js

Lines 237 to 240 in 888f2a4

	"@typescript-eslint/no-unsafe-assignment": "off",
	"@typescript-eslint/no-unsafe-member-access": "off",
	"@typescript-eslint/no-unsafe-return": "off",
	"@typescript-eslint/no-unsafe-call": "off",

Enable `no-non-null-assertion` (?)

Graham mentioned this rule in a review in pixiebrix/pixiebrix-extension#6534, I thought I'd open an issue here to discuss it and potentially add it to this config.

https://typescript-eslint.io/rules/no-non-null-assertion

I'm not entirely sure we should enable it. It's ideal in theory, but very often code guarantees that a value is not null in a way that TypeScript can't verify. Or maybe the types are just too broad (e.g. we know paragraphEl.textContent exists but its type is string | undefined)

In that case we have 3 options:

• add a condition, e.g. if (!x), return or throw, writing code that will never be run
• use ?., pushing the non-existent null down the line
• use !, silence the rule, write explanation

All of these make the code more verbose for non-existent benefit. Maybe the last one works as documentation, but it's still noisy and a cause of additional time spent on an unimportant piece of code.

cc @grahamlangford @mnholtz

Migrate new rules from Extension to Config

The app still has an open PR with the last rules we added so we can't do it too soon:

• https://github.com/pixiebrix/pixiebrix-app/pull/4867

Ban "Omit" in favor of "Except"

• We should always use Except vs. Omit where possible because Except enforces the property actually exists on the object: https://github.com/sindresorhus/type-fest/blob/main/source/except.d.ts
• Might be able to configure on the ban types rule? https://typescript-eslint.io/rules/ban-types/

Move some `no-restricted-syntax` rules from the extension to the shared config

https://github.com/pixiebrix/pixiebrix-extension/blob/0828deb2d7acac35bffcfefaf239015b13a552f4/.eslintrc.js#L87

For example

      {
        message: "Use `jest.mocked(fn)` instead of `fn as jest.Mock`.",
        selector: "TSAsExpression TSQualifiedName[right.name='Mock']",
      },
      {
        message:
          "Use `jest.mocked(fn)` instead of `fn as jest.MockedFunction`.",
        selector: "TSAsExpression TSQualifiedName[right.name='MockedFunction']",
      },