Skip to content
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.

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

eslint-import-resolver-node's resolution does not match node's with respect to symlinks #3110

Open
anomiex opened this issue Nov 28, 2024 · 1 comment

Comments

@anomiex
Copy link

anomiex commented Nov 28, 2024

Consider this example project, structured similarly to what pnpm produces: t.tar.gz

The directory structure looks something like this:

t/
t/index.js
t/node_modules/
t/node_modules/b -> .pnpm/[email protected]/node_modules/b
t/node_modules/.pnpm/
t/node_modules/.pnpm/[email protected]/
t/node_modules/.pnpm/[email protected]/node_modules/
t/node_modules/.pnpm/[email protected]/node_modules/b/
t/node_modules/.pnpm/[email protected]/node_modules/b/index.js
t/node_modules/.pnpm/[email protected]/node_modules/b/package.json
t/node_modules/.pnpm/[email protected]/node_modules/c -> ../../[email protected]/node_modules/c
t/node_modules/.pnpm/[email protected]/
t/node_modules/.pnpm/[email protected]/node_modules/
t/node_modules/.pnpm/[email protected]/node_modules/c/
t/node_modules/.pnpm/[email protected]/node_modules/c/index.js
t/node_modules/.pnpm/[email protected]/node_modules/c/package.json
t/package.json

Of particular note, package b depends on c, and pnpm structures things as above so that t/index.js won't see c as a phantom dependency. c exports a named export 'foo', and b rexports that with export * from 'c'.

When t/index.js imports b, standard Node module resoluton resolves that via the symlink t/node_modules/b, returning the absolute realpath /some/absolute/path/t/node_modules/.pnpm/[email protected]/node_modules/b/index.js. Resolving c relative to that path finds it via the symlink t/node_modules/.pnpm/[email protected]/node_modules/c.

eslint-import-resolver-node's resolution differs in one key respect: by default, instead of returning the absolute realpath /some/absolute/path/t/node_modules/.pnpm/[email protected]/node_modules/b/index.js, it returns /some/absolute/path/t/node_modules/b/index.js instead. It then (correctly) cannot resolve c relative to that path.

A relevant eslint.config.js to use with the above could look like this:

import importPlugin from 'eslint-plugin-import';

export default [ importPlugin.flatConfigs.recommended ];

which will produce the following false positive due to this bug

  1:10  error  foo not found in 'b'  import/named

If you want a "real" reproduction, starting in an empty project:

  1. Create a package.json with "type": "module" (or use extension .mjs instead of .js in steps 3 and 4).
  2. Run pnpm add eslint eslint-plugin-import @playwright/test.
  3. Create the above eslint.config.js.
  4. Create a JS file containing import { expect } from '@playwright/test';.
  5. Run eslint on that file.

As for a fix, it looks like setting preserveSymlinks: false in

function opts(file, config, packageFilter) {
return Object.assign({ // more closely matches Node (#333)
// plus 'mjs' for native modules! (#939)
extensions: ['.mjs', '.js', '.json', '.node'],
}, config, {
// path.resolve will handle paths relative to CWD
basedir: path.dirname(path.resolve(file)),
packageFilter,
});
}
should do it. As a workaround this can be set in the eslint config with

{
    settings: {
        'import/resolver': {
            node: {
                preserveSymlinks: false,
            },
        }
    }
}

I note the README for the resolve package you use recommends setting preserveSymlinks: false to match Node's behavior; as far as I can tell, preserving symlinks was only the default in Node for a short time between 6.0.0 and 6.2.0, although the documentation as to the expected behavior was unclear before that.

@ljharb
Copy link
Member

ljharb commented Nov 28, 2024

jestjs/jest#5356 (comment) has some more of the context/history as well.

You are correct that this value should be set to false by default. The reason that we don't do that in the node resolver at the moment is because that would be a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants