Fixed pre-commit ESLint configuration.

[adamchainz]

Trac ticket number

N/A

Branch description

Broken in #18133 because the mirrors-eslint package does not depend on @eslint/js or globals, leading to the error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'globals' imported from /.../django/eslint.config.mjs

The simplest solution is to instead use a local hook, as done in this PR. This will require manual updates, but that’s the same as we have for the configuration in package.json.

(I put a lot of time into researching options for this upgrade, for my recommendations in Boost Your Django DX. I don’t think the mirrors-eslint repository is going to be usable because there are various packages one wants to pull in for eslint now.)

Tested with:

$ pre-commit run eslint -a
eslint...................................................................Passed

Also ensured that failures would work by patching eslint.config.mjs to require weird indentation:

@@ -11,7 +11,7 @@ export default [
             "curly": ["error", "all"],
             "dot-notation": ["error", {"allowKeywords": true}],
             "eqeqeq": ["error"],
-            "indent": ["error", 4],
+            "indent": ["error", 7],
             "key-spacing": ["error", {"beforeColon": false, "afterColon": true}],
             "linebreak-style": ["error", "unix"],
             "new-cap": ["off", {"newIsCap": true, "capIsNew": true}],

This made ESLint report many errors:

$ pre-commit run eslint -a
eslint...................................................................Failed
- hook id: eslint
- exit code: 1


/.../django/django/contrib/admin/static/admin/js/inlines.js
   20:1  error  Expected indentation of 7 spaces but found 4    indent
   ...

Checklist

• This PR targets the main branch.
• The commit message is written in past tense, mentions the ticket number, and ends with a period.
• I have checked the "Has patch" ticket flag in the Trac system.
• I have added or updated relevant tests.
• I have added or updated relevant docs, including release notes if applicable.
• I have attached screenshots in both light and dark modes for any UI changes.

[felixxm]

Is it not an issue in pre-commit since dependencies are defined in package.json? 🤔

[felixxm]

It seems that it's documented that you need to explicitly define dependencies in the pre-commit configuration.

[adamchainz]

pre-commit uses isolated environments, it never installs from your dependency files like package.json.

[sarahboyce]

@adamchainz can you explain how this resolves the issue?
The way I understand the issue is that you now need to run npm install for the commit hook to work as this installs globals as a dev dependency. With this change, if I run npm uninstall globals and run this, I get the same errors

One way forward we could consider would be to remove our own dependency to the globals package and defining our globals in eslint.config.js

[adamchainz]

That is not the issue. npm install does not affect pre-commit. (If you switch to main and run pre-commit run eslint -a then you’ll see it fails regardless of whether node_modules is set up.)

pre-commit is its own package manager, installing tools in isolated environments, ignoring any configuration in your repository except .pre-commit-config.yaml.

In this case, pre-commit was installing the packages listed in the mirrors-eslint repo: https://github.com/pre-commit/mirrors-eslint/blob/main/.pre-commit-hooks.yaml - only eslint, not @eslint/js or globals. Because our config requires both, it would always fail.

To get pre-commit to install other packages, we need to list them in its additional_depedencies option. This overrides the eslint-mirror repo’s additional_dependencies. In doing so, we lose the advantage of using the mirror repo, which really only exists to automatically bump the eslint@9.2.0 declaration. So we can move to a local repo declaration, where we list all the configuration necessary to install and run eslint.

[sarahboyce]

pre-commit run eslint -a passes on main for me

[adamchainz]

Ah, you know what, I was mistaken. I was acting based on my experience of one of the ESLint beta or RC releases requiring this change, rather than testing here. Yes, it works on main, because ESLint depends on both @eslint/js and globals: https://github.com/eslint/eslint/blob/main/package.json . Sorry for the noise.

[carltongibson]

Relevant maybe here: ESLint are asking for feedback on the v9 migration. Maybe folks have thoughts.

eslint/eslint#18456

🎁

[adamchainz]

Thanks, Carlton, but the problem here was me :)

(Now, to fix my book...)

[adamchainz]

No wait, I’m not crazy. pre-commit run eslint -a fails on main if you haven’t run npm install. Somehow the way that ESLint loads the configuration makes it use node_modules for the imports, rather its own repository. (Working on a bug report there...)

[smithdc1]

This may be related -- ekalinin/nodeenv#353

[adamchainz]

Thanks David. I made a bug report to ESLint: eslint/eslint#18465

[sritchie]

I spent some time looking into this issue as well for a private project. Here are my notes, if they help.

I found what you all did:

• pre-commit is correctly installing additional dependencies
• when the entry: function is called, $NPM_CONFIG_PREFIX is correctly set the directory containing the pre-commit environment's bin, lib etc
• eslint ignores this prefix and only looks at the current directory where eslint is running to find node_modules.

This problem is independent of whether or not we use local or the mirrored eslint.

It seems that eslint used to take a --resolve-plugins-relative-to flag, but no longer does. I also found reference around the web to a config flag of resolvePluginsRelativeTo, but every attempt I made at using this key in eslint.config.js in my project failed (I wasn't able to get eslint to print the kv pair back out when running

eslint --print-config eslint.config.js

I don't think the eslint authors understand the goal here, based on the reply to eslint/eslint#18465, but I DO agree that this issue makes it impossible to use pre-commit.ci with eslint, and is a real problem.