Skip to content

feat: old import-x/resolve options now use new built-in node resolver #272

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.

Sign up for GitHub

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

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

feat: old import-x/resolve options now use new built-in node resolver #272

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
85a9702
feat: old `import-x/resolve` options now use new built-in node resolver
SukkaW Mar 30, 2025
89e3937
Update src/utils/resolve.ts
SukkaW Mar 30, 2025
2a66b0a
refactor: address a few suggestions
SukkaW Mar 30, 2025
ffcc7d3
fix: esm
SukkaW Mar 30, 2025
161c7cc
fix: short legacy resolver instance var name
SukkaW Mar 30, 2025
28189c5
fix: remove object has own check
SukkaW Mar 30, 2025
2b0fce1
chore: move `eslint-import-resolver-node` to devDeps
SukkaW Mar 31, 2025
05fcf87
Update src/types.ts
SukkaW Apr 1, 2025
d21b8d8
fix: try adding paths to modules
SukkaW Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@
"@typescript-eslint/utils": "^8.28.0",
"debug": "^4.4.0",
"doctrine": "^3.0.0",
"eslint-import-resolver-node": "^0.3.9",
"get-tsconfig": "^4.10.0",
"is-glob": "^4.0.3",
"minimatch": "^10.0.1",
Expand Down Expand Up @@ -129,6 +128,7 @@
"eslint": "^9.23.0",
"eslint-config-prettier": "^10.1.1",
"eslint-doc-generator": "^2.1.2",
"eslint-import-resolver-node": "^0.3.9",
"eslint-import-resolver-typescript": "^4.3.1",
"eslint-import-resolver-webpack": "^0.13.10",
"eslint-import-test-order-redirect": "link:./test/fixtures/order-redirect",
Expand Down
29 changes: 26 additions & 3 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,33 @@ export type {

export type ImportType = ImportType_ | 'object' | 'type'

export interface NodeResolverOptions {
export interface LegacyNodeResolverOptions {
extensions?: readonly string[]
moduleDirectory?: string[]
/** set to `false` to exclude node core modules (e.g.` fs`) from the search */
includeCoreModules?: boolean
/** directory (or directories) in which to recursively look for modules. Default "node_modules" */
moduleDirectory?: string
/** 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 */
preserveSymlinks?: boolean
/** 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[]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@JounQin JounQin Mar 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

image

I don't think basedir is exposed.

image

modules is the replacement of paths.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

modules is the replacement of paths.

But resolve also has moduleDirectory. IMHO the paths here aligns with require.paths.

Copy link
Collaborator Author

@SukkaW SukkaW Mar 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 } }
];

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But we will still need to implement paths, otherwise the unit tests will fail.

Copy link
Member

Choose a reason for hiding this comment

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

modules can not cover paths test cases? Then I'd like to still mark resolver-node as optional peer.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't get it. I just tried adding paths to modules. The tests are still failing.


/** Noop now. Previously for how to read files asynchronously */
readFile?: never
/** Noop now. Previously a function to asynchronously test whether a file exists */
isFile?: never
/** Noop now. Previously a function to asynchronously whether a file exists and is a directory */
isDirectory?: never
/** Noop now. Previously a function to asynchronously resolve a potential symlink to its real path */
realpath?: never
/** Noop now. Previously a function to asynchronously read a package.json file */
readPackage?: never
/** Noop now. Previously a function to transform the parsed package.json contents before looking at the "main" field */
packageFilter?: never
/** Noop now. Previously a function to transform the resolved path before returning it */
pathFilter?: never
}

export interface WebpackResolverOptions {
Expand Down Expand Up @@ -98,7 +121,7 @@ export interface ImportSettings {
ignore?: string[]
internalRegex?: string
parsers?: Record<string, readonly FileExtension[]>
resolve?: NodeResolverOptions
resolve?: LegacyNodeResolverOptions
resolver?: LegacyImportResolver
'resolver-legacy'?: LegacyImportResolver
'resolver-next'?: NewResolver[]
Expand Down
6 changes: 3 additions & 3 deletions src/utils/legacy-resolver-settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { cjsRequire } from '@pkgr/core'
import type { LiteralUnion } from 'type-fest'

import type {
NodeResolverOptions,
LegacyNodeResolverOptions,
ResolvedResult,
TsResolverOptions,
WebpackResolverOptions,
Expand Down Expand Up @@ -48,7 +48,7 @@ export interface LegacyResolverObject {

// Options passed to the resolver
options?:
| NodeResolverOptions
| LegacyNodeResolverOptions
| TsResolverOptions
| WebpackResolverOptions
| unknown
Expand All @@ -58,7 +58,7 @@ export interface LegacyResolverObject {
}

export interface LegacyResolverRecord {
node?: boolean | NodeResolverOptions
node?: boolean | LegacyNodeResolverOptions
typescript?: boolean | TsResolverOptions
webpack?: WebpackResolverOptions
[resolve: string]: unknown
Expand Down
76 changes: 48 additions & 28 deletions src/utils/resolve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { fileURLToPath } from 'node:url'

import { stableHash } from 'stable-hash'

import { createNodeResolver } from '../node-resolver.js'
import type {
ImportSettings,
LegacyResolver,
Expand Down Expand Up @@ -110,6 +111,8 @@ function isValidNewResolver(resolver: unknown): resolver is NewResolver {
return true
}

let fallbackLegacyNodeResolver: NewResolver

function fullResolve(
modulePath: string,
sourceFile: string,
Expand Down Expand Up @@ -140,10 +143,7 @@ function fullResolve(
return { found: true, path: cachedPath }
}

if (
Object.prototype.hasOwnProperty.call(settings, 'import-x/resolver-next') &&
settings['import-x/resolver-next']
) {
if (settings['import-x/resolver-next']) {
const configResolvers = settings['import-x/resolver-next']

for (let i = 0, len = configResolvers.length; i < len; i++) {
Expand All @@ -169,33 +169,53 @@ function fullResolve(
fileExistsCache.set(cacheKey, resolved.path as string | null)
return resolved
}
} else {
const configResolvers = settings['import-x/resolver-legacy'] ||
settings['import-x/resolver'] || {
node: settings['import-x/resolve'],
} // backward compatibility

for (const { enable, options, resolver } of normalizeConfigResolvers(
configResolvers,
sourceFile,
)) {
if (!enable) {
continue
}
} else if (
Copy link
Member

Choose a reason for hiding this comment

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

It seems not covering import-x/resolver#node, import-resolver#eslint-import-resolver-node and , import-x/resolver#<absolute-path-of-resolver-node>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

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.

Copy link
Member

@JounQin JounQin Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmmm. You are right. Let's move this to the breaking change then, and no legacy options will be supported. Closing now.

Copy link
Member

Choose a reason for hiding this comment

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

I still want to support it with my proposal, so I'll continue your work to finish it.

// backward compatibility for very old `import-x/resolve` options that is no longer supported
settings['import-x/resolve']
) {
const resolveSettings = settings['import-x/resolve']

fallbackLegacyNodeResolver ||= createNodeResolver({
extensions: (resolveSettings.extensions ||
settings['import-x/extensions']) as string[] | undefined,
builtinModules: resolveSettings.includeCoreModules !== false,
modules: resolveSettings.moduleDirectory,
symlinks: resolveSettings.preserveSymlinks ?? true,
})

const resolved = fallbackLegacyNodeResolver.resolve(modulePath, sourceFile)
if (resolved.found) {
fileExistsCache.set(cacheKey, resolved.path as string | null)
return resolved
}

const resolved = resolveWithLegacyResolver(
resolver,
options,
modulePath,
// not found, don't do anything
} else {
const configResolvers =
settings['import-x/resolver-legacy'] || settings['import-x/resolver']
if (configResolvers) {
for (const { enable, options, resolver } of normalizeConfigResolvers(
configResolvers,
sourceFile,
)
if (!resolved.found) {
continue
}
)) {
if (!enable) {
continue
}

const resolved = resolveWithLegacyResolver(
resolver,
options,
modulePath,
sourceFile,
)
if (!resolved.found) {
continue
}

// else, counts
fileExistsCache.set(cacheKey, resolved.path as string | null)
return resolved
// else, counts
fileExistsCache.set(cacheKey, resolved.path as string | null)
return resolved
}
}
}

Expand Down
Loading