[New] add requireResolve option to check require.resolve() paths - #3264
[New] add requireResolve option to check require.resolve() paths#3264manzoorwanijk wants to merge 7 commits into
requireResolve option to check require.resolve() paths#3264Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3264 +/- ##
==========================================
+ Coverage 79.57% 79.66% +0.08%
==========================================
Files 98 98
Lines 4539 4559 +20
Branches 1537 1553 +16
==========================================
+ Hits 3612 3632 +20
Misses 927 927 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@ljharb, a gentle request: is it possible to wrap this up before the next release? |
e298b1f to
933e918
Compare
|
Some CI failures are due to git being blocked by npm v12 by default - https://github.com/import-js/eslint-plugin-import/actions/runs/29027018826/job/86149376901?pr=3264 |
933e918 to
35a18db
Compare
35a18db to
cd994d0
Compare
There was a problem hiding this comment.
Pull request overview
Adds opt-in support for treating require.resolve("<specifier>") as a resolvable module specifier across relevant eslint-plugin-import rules, via a new shared requireResolve boolean option (default false) on the common moduleVisitor.
Changes:
- Extend
moduleVisitorto optionally visitrequire.resolve()string-literal arguments (skipping non-string and multi-arg forms). - Wire the new option into additional rules’ schemas/visitor options (and explicitly ignore
require.resolveas a cycle edge inno-cycle). - Add/adjust tests and documentation for the impacted rules, plus a changelog entry.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| utils/moduleVisitor.js | Adds requireResolve option and require.resolve() detection to the shared visitor + schema. |
| utils/moduleVisitor.d.ts | Adds requireResolve?: boolean to the visitor options type. |
| src/rules/no-webpack-loader-syntax.js | Adds rule option schema + forwards requireResolve into moduleVisitor. |
| src/rules/no-useless-path-segments.js | Extends schema to allow requireResolve (forwarded via existing options pass-through). |
| src/rules/no-self-import.js | Adds rule option schema + forwards requireResolve into moduleVisitor. |
| src/rules/no-internal-modules.js | Extends schema + forwards requireResolve into moduleVisitor. |
| src/rules/no-extraneous-dependencies.js | Extends schema + forwards requireResolve into moduleVisitor. |
| src/rules/no-cycle.js | Ensures visited require.resolve() paths are ignored as cycle edges. |
| tests/src/rules/no-webpack-loader-syntax.js | Adds valid/invalid coverage for require.resolve() when option is enabled. |
| tests/src/rules/no-useless-path-segments.js | Adds coverage for auto-fixing require.resolve() paths when enabled. |
| tests/src/rules/no-unresolved.js | Adds valid/invalid coverage for require.resolve() resolution behavior and ignored forms. |
| tests/src/rules/no-self-import.js | Adds coverage for self-import via require.resolve() gated behind the option. |
| tests/src/rules/no-internal-modules.js | Adds coverage for internal-module reaching via require.resolve() gated behind the option. |
| tests/src/rules/no-extraneous-dependencies.js | Adds coverage for dependency checks on require.resolve() gated behind the option. |
| tests/src/rules/no-cycle.js | Adds regression coverage ensuring require.resolve() does not create cycle edges. |
| tests/src/rules/no-absolute-path.js | Adds coverage for absolute-path checks on require.resolve() gated behind the option. |
| docs/rules/no-webpack-loader-syntax.md | Documents the new requireResolve option. |
| docs/rules/no-useless-path-segments.md | Documents the new requireResolve option. |
| docs/rules/no-unresolved.md | Documents the new requireResolve option + ignored paths form / non-string forms. |
| docs/rules/no-self-import.md | Documents the new requireResolve option. |
| docs/rules/no-internal-modules.md | Documents the new requireResolve option. |
| docs/rules/no-extraneous-dependencies.md | Documents the new requireResolve option. |
| docs/rules/no-absolute-path.md | Adds requireResolve to the shared options list. |
| CHANGELOG.md | Notes the addition of requireResolve and the affected rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
afc9ce2 to
6173bd6
Compare
…ve()` paths
Adds an opt-in `requireResolve` boolean (default false) to the shared module
visitor and its options schema. When enabled, single- or multi-arg
`require.resolve("<literal>")` calls have their first string argument visited as
a `require` module path, so resolution-based rules can check it.
…solve` Resolves import-js#585. The shared `requireResolve` option (off by default) makes `no-unresolved` report unresolvable `require.resolve("./foo")` paths.
`require.resolve` computes a path without loading the module, so it cannot form a runtime dependency cycle. Skip it in the visitor so enabling `requireResolve` does not produce false cycle reports.
…ia `requireResolve`
Adds the `requireResolve` option to the rule schema and forwards it to the
module visitor, so a package pulled in via `require.resolve("pkg")` is flagged
when it is not a declared dependency.
…segments`, `no-internal-modules`: check `require.resolve()` paths via `requireResolve` Wires the opt-in `requireResolve` option into the remaining path-checking rules that resolve module specifiers, so each also inspects `require.resolve()` paths.
6173bd6 to
fd48366
Compare
fd48366 to
ac2d993
Compare
Closes #585. Supersedes #1217.
Motivation
While working on a PR in Gutenberg (which powers the WordPress block editor), I noticed that
require.resolve()calls are not considered by these rules. Looking for existing discussion I found #585, so this PR implements it.Summary
require.resolve()resolves a module path the same wayrequire()does, but until now the resolution rules ignored it. This PR adds an opt-inrequireResolveboolean option (defaultfalse) to the sharedmoduleVisitor. When enabled, the first string-literal argument of arequire.resolve("<path>")call is visited as arequiremodule path, so the rules that resolve specifiers can check it too.Relation to #1217
This supersedes the earlier #1217, which also added a
requireResolveoption but stalled in review. The main differences here: the option is a plain boolean independent ofcommonjs(instead of theboolean | { commonjs }oneOf), the 2-arg{ paths }form is ignored since resolvers cannot honor a custom resolution base,no-cycleexplicitly ignoresrequire.resolve, and the option is wired into several additional resolution rules — all with tests and docs.Why opt-in (off by default)
This mirrors the conclusion of the earlier attempts (#1216 / #1217): emitting new warnings on existing code is a breaking change, and
require.resolveis sometimes used to resolve non-module assets (e.g. build-generated files) that a resolver can't find. The option therefore defaults tofalseand is independent ofcommonjs.Affected rules
The change lives in
moduleVisitor, so every resolution rule can opt in:no-unresolved], [no-absolute-path], [no-relative-packages], [no-relative-parent-imports].no-cycle]: explicitly ignoresrequire.resolve— it computes a path without loading the module, so it cannot form a runtime cycle. This prevents false cycle reports when the option is enabled elsewhere.no-extraneous-dependencies], [no-self-import], [no-webpack-loader-syntax], [no-useless-path-segments], [no-internal-modules].Deliberately not wired:
max-dependencies(a resolved path is not a load-time dependency edge),extensions(require.resolveis often used specifically to resolve paths with an extension), andno-nodejs-modules(marginal value).Behaviour / edge cases
require.resolve(path, { paths })form is ignored:pathsoverrides the resolution base, which resolvers cannot honor, so checking the first argument would produce false positives. (This deviates from the "check the first argument in every case" suggestion in the Add requireResolve option #1217 review, which predates thepathssemantics being raised.)require.resolve(...)member calls match;require['resolve'](...),require[resolve](...), andfoo.require.resolve(...)are ignored.moduleSystem: 'require', so resolvermoduleSystemconfiguration applies consistently withrequire().Checklist
docs/rules/*)Testing
npm run tests-onlyfor the affected suites — all passingeslint .,tsc --noEmit index.d.ts,npm run update:eslint-docs -- --check, andmarkdownlintall cleanAI tool disclosure
This change was prepared with AI assistance. The plan, implementation, tests, and docs were drafted with Claude Code, and the plan and final diff were independently reviewed with Codex. All output was reviewed by me before submission, and I take responsibility for the contents of this PR.