feat: old import-x/resolve options now use new built-in node resolver
#272
Conversation
|
WalkthroughThis pull request restructures the node resolver configuration by replacing Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/utils/resolve.tsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/types.tsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the legacy "import-x/resolve" option to use the new built-in Node resolver, improving consistency and removing unsupported legacy options.
- Updates the resolution logic in src/utils/resolve.ts to utilize the new node resolver for backward compatibility.
- Renames type definitions from NodeResolverOptions to LegacyNodeResolverOptions in both src/utils/legacy-resolver-settings.ts and src/types.ts.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/utils/resolve.ts | Implements new resolution logic using createNodeResolver and updates branch logic for legacy settings. |
| src/utils/legacy-resolver-settings.ts | Updates type imports to use LegacyNodeResolverOptions for legacy resolver configurations. |
| src/types.ts | Renames NodeResolverOptions to LegacyNodeResolverOptions and updates associated documentation. |
Comments suppressed due to low confidence (2)
src/utils/resolve.ts:107
- [nitpick] The variable name 'nodeResolverInstanceForLegacyNodeResolverSettings' is overly verbose. Consider renaming it to 'legacyNodeResolver' or a similar concise identifier.
let nodeResolverInstanceForLegacyNodeResolverSettings: NewResolver | null = null
src/utils/resolve.ts:168
- The new branch for handling legacy 'import-x/resolve' options should be accompanied by test cases to validate its behavior. Consider adding tests that cover both successful and unsuccessful resolution scenarios.
else if ( Object.prototype.hasOwnProperty.call(settings, 'import-x/resolve') && settings['import-x/resolve'] ) {
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
commit: |
src/types.ts
Outdated
| /** Noop now, Previously a directory to begin resolving from */ | ||
| basedir?: string | ||
| /** Noop now. Previously for require.paths array to, it is now noop */ | ||
| paths?: string[] |
There was a problem hiding this comment.
Both the basedir option and the paths option are not supported by unrs-resolver. Is it possible to implement this, or should we just make it as noop?
This affects many unit tests IMHO.
There was a problem hiding this comment.
I don't think basedir is exposed.
modules is the replacement of paths.
There was a problem hiding this comment.
IMO, we can mark eslint-import-resolver-node as optional peer dependency, if the user passed options we didn't support, we use original eslint-import-resolver-node, otherwise, use our internal node resolver instead.
There was a problem hiding this comment.
modulesis the replacement ofpaths.
But resolve also has moduleDirectory. IMHO the paths here aligns with require.paths.
There was a problem hiding this comment.
IMO, we can mark
eslint-import-resolver-nodeas optional peer dependency, if the user passed options we didn't support, we use originaleslint-import-resolver-node, otherwise, use our internal node resolver instead.
The idea is that almost nobody uses import/resolve:
https://github.com/search?q=%2F%5B%27%22%5Dimport%5C%2Fresolve%5B%27%22%5D%2F&type=code
And only our unit tests use import-x/resolve:
https://github.com/search?q=%2F%5B%27%22%5Dimport-x%5C%2Fresolve%5B%27%22%5D%2F&type=code
This is because the import/resolve has been "deprecated" long ago and has never been documented since. But this is only part that requires eslint-import-resolver-node.
So we can probably include this as a semi-breaking change and drop eslint-import-resovler-node completely without a non-major version bump.
There was a problem hiding this comment.
IMHO, let's just remove it first, and probably no eslint-plugin-import-x users will be affected.
Implementing fallback will result in inconsistent behaviors (sometimes use eslint-import-resolver-node, sometimes use unrs-resolver), there might be this:
// eslint.config.js
export default [
{ settings: { 'import-x/resolve': legacy } },
{ settings: { 'import-x/resolve': lessLegacy } }
];There was a problem hiding this comment.
But we will still need to implement paths, otherwise the unit tests will fail.
There was a problem hiding this comment.
modules can not cover paths test cases? Then I'd like to still mark resolver-node as optional peer.
There was a problem hiding this comment.
opts.paths - require.paths array to use if nothing is found on the normal node_modules recursive walk (probably don't use this)
I think paths behavior can be mocked when found: false and do another try through modules: paths?
There was a problem hiding this comment.
I don't get it. I just tried adding paths to modules. The tests are still failing.
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
SukkaW
force-pushed
the
replace-eslint-import-resolver-node
branch
from
7dab322 to
2a66b0a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
src/utils/resolve.ts (1)
114-114: Variable initialization concernThis variable is declared but only conditionally initialized. Consider using
let nodeResolverInstanceForLegacyNodeResolverSettings: NewResolver | undefinedto make nullability explicit.-let nodeResolverInstanceForLegacyNodeResolverSettings: NewResolver +let nodeResolverInstanceForLegacyNodeResolverSettings: NewResolver | undefined🧰 Tools
🪛 GitHub Actions: Publish Any Commit
[error] Circular dependency: src/utils/resolve.ts -> src/utils/legacy-resolver-settings.ts -> src/utils/resolve.ts
src/types.ts (1)
45-48: Document the reason for noop propertiesProperties marked as "Noop now" should have a clearer explanation of why they've been deprecated or marked as noop, especially since they were previously functional as indicated by the comments.
- /** Noop now, Previously a directory to begin resolving from */ + /** Noop now. Previously a directory to begin resolving from. These legacy options are maintained for backward compatibility but no longer affect resolution. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/types.ts(2 hunks)src/utils/legacy-resolver-settings.ts(3 hunks)src/utils/resolve.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/utils/resolve.ts (2)
src/types.ts (1)
NewResolver(86-91)src/utils/legacy-resolver-settings.ts (2)
normalizeConfigResolvers(103-155)resolveWithLegacyResolver(75-101)
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on macos-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Check: publish
src/types.ts
[failure] 124-124:
Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/legacy-resolver-settings.ts
[failure] 10-10:
'"../types.js"' has no exported member named 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[failure] 7-7:
Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../node-resolver.js'?
🪛 GitHub Actions: Publish Any Commit
src/types.ts
[error] 124-124: error TS2552: Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[warning] 24-24: "import.meta" is not available in the configured target environment ("es2015") and will be empty [empty-import-meta]
[error] Circular dependency: src/utils/resolve.ts -> src/utils/legacy-resolver-settings.ts -> src/utils/resolve.ts
🪛 GitHub Actions: CI
src/types.ts
[error] 124-124: error TS2552: Cannot find name 'LegacyNodeResolverOptions'. Did you mean 'NodeResolverOptions'?
src/utils/resolve.ts
[warning] 24-24: "import.meta" is not available in the configured target environment ("es2015") and will be empty
🔇 Additional comments (3)
src/utils/resolve.ts (2)
175-181: Consider extracting settings to a variableThe settings['import-x/resolve'] is accessed multiple times. Consider extracting it to a variable for better readability and maintainability.
- } else if ( - // backward compatibility for very old `import-x/resolve` options that is no longer supported - Object.prototype.hasOwnProperty.call(settings, 'import-x/resolve') && - settings['import-x/resolve'] - ) { - const resolveSettings = settings['import-x/resolve'] + } else if ( + // backward compatibility for very old `import-x/resolve` options that is no longer supported + Object.prototype.hasOwnProperty.call(settings, 'import-x/resolve') && + settings['import-x/resolve'] + ) { + const resolveSettings = settings['import-x/resolve']This was already suggested in a previous review and implemented in a later commit.
🧰 Tools
🪛 GitHub Actions: Publish Any Commit
[error] Circular dependency: src/utils/resolve.ts -> src/utils/legacy-resolver-settings.ts -> src/utils/resolve.ts
182-188: Verify semantic equivalence between optionsThere may be a semantic mismatch between
includeCoreModules(legacy) andbuiltinModules(new API).- builtinModules: resolveSettings.includeCoreModules !== false, + builtinModules: resolveSettings.includeCoreModules !== false,This was flagged in a previous review. If
includeCoreModules: falsemeans "exclude core modules" in the legacy API, verify thatbuiltinModules: falsehas the same meaning in the new API.What's the difference between builtinModules and includeCoreModules in Node.js module resolution?🧰 Tools
🪛 GitHub Actions: Publish Any Commit
[error] Circular dependency: src/utils/resolve.ts -> src/utils/legacy-resolver-settings.ts -> src/utils/resolve.ts
src/types.ts (1)
42-42:✅ Verification successful
Verify moduleDirectory type compatibility
The type of
moduleDirectoryhas been changed from what was likely an array to a single string. Make sure this is compatible with how it's used in the resolution logic.
🏁 Script executed:
#!/bin/bash # Check for usages of moduleDirectory in the codebase echo "Checking how moduleDirectory is used:" rg "moduleDirectory" --include "*.ts" .Length of output: 566
Action Required: Verify the
moduleDirectoryType UsageThe
moduleDirectoryfield insrc/types.tshas been changed from an array to a string. The initial search using--includeresulted in an error, so please re-run the query with the corrected flag to check howmoduleDirectoryis used throughout the code. For example, you can use the following script:Once you verify that all usages of
moduleDirectoryexpect a string (and not an array), you can confirm that the resolution logic remains correct. If any usage still treats it as an array, additional adjustments may be required.
🏁 Script executed:
#!/bin/bash echo "Searching for usages of moduleDirectory in .ts files:" rg "moduleDirectory" -g "*.ts" .Length of output: 384
Confirmed: moduleDirectory Type Compatibility is Consistent
Based on the verification:
- In
src/types.ts,moduleDirectoryis defined as an optional string.- In
src/utils/resolve.ts,moduleDirectoryis used by simply passing it to themodulesfield without any array-specific operations.- No evidence was found that any part of the resolution logic still expects
moduleDirectoryto be an array.The change from an array to a string appears compatible with the current usage in the resolution logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/types.ts (1)
45-48: Consider adding modules parameter supportBased on the previous review comments, there's a discussion about
basedirandpaths. It appears thatmodulesis the replacement forpathsin the new resolver.Consider adding a comment about how users can migrate from
pathsto the modern equivalent in the new resolver, if applicable. This would help with the migration and backwards compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/types.ts(2 hunks)src/utils/resolve.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/resolve.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
🔇 Additional comments (4)
src/types.ts (4)
37-64: Interface name change aligns with migration to built-in resolverThe renaming from
NodeResolverOptionstoLegacyNodeResolverOptionsclearly indicates this is for backward compatibility while the codebase migrates to the new built-in node resolver. The addition of properties marked as "Noop now" maintains the interface signature while properly documenting which options are no longer functional.
50-63: Effective use of the never type for no-op propertiesUsing the
nevertype for deprecated properties is a good practice. It ensures TypeScript will raise an error if someone tries to use these properties, while still maintaining the interface shape for backward compatibility.
124-124: Property correctly updated to use new interface nameThe property type has been correctly updated to reference the renamed interface, fixing the build failures noted in the previous review comments.
41-42:❓ Verification inconclusive
Comment and type are inconsistent for moduleDirectory
The comment mentions "directory (or directories)" but the type has been changed from
string[]tostring, which only allows a single directory.- /** directory (or directories) in which to recursively look for modules. Default "node_modules" */ - moduleDirectory?: string + /** directory in which to recursively look for modules. Default "node_modules" */ + moduleDirectory?: stringPlease verify if the built-in resolver supports multiple directories. If it does, consider using
string | string[]to maintain backward compatibility.
🏁 Script executed:
#!/bin/bash # Check if unrs-resolver supports multiple directories for moduleDirectory rg -F "moduleDirectory" --context 10 --type ts node_modules/unrs-resolverLength of output: 73
Action Required: Confirm Multi-Directory Support
The automated search did not reveal any indication that the built-in resolver supports multiple directories for
moduleDirectory. At this point, the change updating the comment to suggest only a single directory (with the type remaining asstring) appears appropriate. However, if the resolver actually supports an array (i.e., multiple directories), the type should be updated tostring | string[]to maintain backward compatibility.
- Location:
src/types.tsat lines 41–42- Next Step: Please manually verify the built-in resolver’s source or documentation to confirm whether it accepts multiple directories for
moduleDirectory. If it does, update the type accordingly; otherwise, the current single-string approach is correct.
| if (!enable) { | ||
| continue | ||
| } | ||
| } else if ( |
There was a problem hiding this comment.
It seems not covering import-x/resolver#node, import-resolver#eslint-import-resolver-node and , import-x/resolver#<absolute-path-of-resolver-node>
There was a problem hiding this comment.
I want to leave those three in the major version bump. These could potentially be a breaking change (unlike import-x/resolve, which almost nobody uses, but we can run tests on it).
There was a problem hiding this comment.
IMHO, they can still use eslint-import-resolver-node explicitly, we should not stop it.
It is the import-x/resolve that is default to eslint-import-resolver-node and we need to replace that.
There was a problem hiding this comment.
resolver-node is the default resolver and doesn't support exports, that's makes a lot of duplicate issues reported in upstream, as I mentioned above, if no specific features used by resolver-node options, we can safely use our new node resolver instead, otherwise we can just fallback to original resolver-node when unsupported options passed.
Then no breaking changes, and no regressions?
There was a problem hiding this comment.
resolver-nodeis the default resolver and doesn't supportexports, that's makes a lot of duplicate issues reported in upstream, as I mentioned above, if no specific features used byresolver-nodeoptions, we can safely use our new node resolver instead, otherwise we can just fallback to originalresolver-nodewhen unsupported options passed.Then no breaking changes, and no regressions?
The only default thing that happened here is in the import-x/resolve (without r); my PR is trying to remove it. There is nothing else that defaults to resolver-node.
Here is the thing, it is possible to have both node resolvers enabled:
// eslint.config.js
export default [
{ files, settings: { 'import-x/resolve': legacy } },
{ files, settings: { 'import-x/resolve': lessLegacy } }
];Then it just becomes "why my exports are not working? I have already updated to the latest version! You are lying in the CHANGELOG!". Or "exports only works times to times!"
Let's just not overcomplicate things. If they explicitly declare node in the import-x/resolver (soon to become import-x/resolver-legacy), then we use resolver-node. Only if nothing is explicitly defined then we use our built-in node resolver by default.
There was a problem hiding this comment.
Removing resolver-node from dep already breaks the related users.
We can warn and document if unsupported options passed, it'll fallback to original resolver-node.
I think resolver-node should be an optional peer.
There was a problem hiding this comment.
Hmmmm. You are right. Let's move this to the breaking change then, and no legacy options will be supported. Closing now.
There was a problem hiding this comment.
I still want to support it with my proposal, so I'll continue your work to finish it.
Co-authored-by: JounQin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/types.ts (3)
47-48: Fix incomplete comment for paths propertyThe comment for the
pathsproperty is incomplete and doesn't fully explain its previous purpose.- /** Noop now. Previously for require.paths array to */ + /** Noop now. Previously for require.paths array to use if nothing is found on the normal node_modules recursive walk */
39-44: Consider adding default values in commentsFor properties like
includeCoreModules,moduleDirectory, andpreserveSymlinks, it would be helpful to include their default values in the comments to provide better guidance for users.- /** set to `false` to exclude node core modules (e.g.` fs`) from the search */ + /** set to `false` to exclude node core modules (e.g.` fs`) from the search. Default: true */ - /** directory (or directories) in which to recursively look for modules. Default "node_modules" */ + /** directory in which to recursively look for modules. Default: "node_modules" */ - /** if true, doesn't resolve basedir to real path before resolving. This is the way Node resolves dependencies when executed with the --preserve-symlinks flag. Default to true */ + /** if true, doesn't resolve basedir to real path before resolving. This is the way Node resolves dependencies when executed with the --preserve-symlinks flag. Default: true */
45-63: Ensure consistency in comment styleThere's a slight inconsistency in the comment style for the noop properties. Some use "Noop now, Previously..." while others use "Noop now. Previously..." (with a period).
Consider standardizing to one style for consistency, preferably "Noop now. Previously..." (with a period) as it's used more frequently in the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/types.ts(2 hunks)src/utils/resolve.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/resolve.ts
🔇 Additional comments (2)
src/types.ts (2)
37-64: LGTM: Interface structure is clear with appropriate documentationThe
LegacyNodeResolverOptionsinterface is well-organized with clear documentation of which options are now no-ops versus those that remain functional. This approach properly maintains backward compatibility while moving to the new built-in node resolver.
124-124: Properly updated type reference in ImportSettingsThis properly updates the type reference to use the new
LegacyNodeResolverOptionsinterface.
| builtinModules: resolveSettings.includeCoreModules !== false, | ||
| modules: [ | ||
| resolveSettings.moduleDirectory, | ||
| ...(resolveSettings.paths ?? []), |
There was a problem hiding this comment.
paths should only be applied when moduleDirectory fails to resolve (found: false).
There was a problem hiding this comment.
Well. Probably just implements this in unrs-resolver TBH.
There was a problem hiding this comment.
That could be confusing between modules vs paths, and I think that's why this is not implemented in enhanced-resolve. paths is also discouraged by resolve itself:
opts.paths-require.pathsarray to use if nothing is found on the normalnode_modulesrecursive walk (probably don't use this)
|
Migrate the legacy
import-x/resolveoptions to use our new built-in node resolver.The
import-x/resolveis a legacy option that is not even documented anymore (neither in theeslint-plugin-importnoreslint-plugin-import-x).Summary by CodeRabbit
"eslint-import-resolver-node": "^0.3.9"in the package configuration.