Improve module resolution

[ngotchac]

Tell us about your environment

• ESLint Version:
  v4.16.0
• Node Version:
  v9.4.0
• npm Version:
  v5.6.0

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

This is a follow-up of #9746.
See this repository, which explains the issue: chrisblossom/eslint-issue-9746

What did you expect to happen?

ESLint should be able to work inside Lerna projects.

What actually happened? Please include the actual, raw output from ESLint.

ESLint throws errors regarding module resolutions.

As described here (#9746 (comment)) it seems that this is the expected behaviour, however I'd argue that the changes presented here: #9896 are actually improving the module resolution logic, while fixing #9746.

First, it splits up the extraLookup path into multiple paths where modules should be looked for, according to Node module specifications.

Secondly, the change to getBaseDir seems pretty straightforward: the projectPath should not be a node_modules directory.

If those changes are accepted, it would actually resolve issues users have with ESLint and Lerna, while improving the module resolution logic.

[not-an-aardvark]

Thanks for creating an issue. I'm still a bit confused about what the proposed change actually does at a high level (I understand the current behavior generally, but I'm not very familiar with the current implementation details such as extraLookup and getBaseDir).

Presumably, if this change allows ESLint to resolve the configs from #9746, then this change is making ESLint search for configs in places where it wasn't looking for them previously. Could you clarify where it tries to look for a config with this change applied?

My goal in asking these questions is to try to make the high-level behavior change clearer, so that we can ensure it makes sense and works well in general aside from the specific case in #9746.

[ngotchac]

Yep, so basically it adds new paths to lookup for module resolution.
For example in https://github.com/chrisblossom/eslint-issue-9746, looking for eslint-config-example

Current lookup paths:

[ '/home/nicolas/Playground/eslint-issue-9746/node_modules/node_modules',
  '/home/nicolas/Playground/eslint-issue-9746/node_modules/eslint/lib/util/node_modules',
  '/home/nicolas/Playground/eslint-issue-9746/node_modules/eslint/lib/node_modules',
  '/home/nicolas/Playground/eslint-issue-9746/node_modules/eslint/node_modules',
  '/home/nicolas/Playground/eslint-issue-9746/node_modules',
  '/home/nicolas/Playground/node_modules',
  '/home/nicolas/node_modules',
  '/home/node_modules',
  '/node_modules',
  '/home/nicolas/.node_modules',
  '/home/nicolas/.node_libraries',
  '/usr/lib/node' ]

With this PR

[ '/home/nicolas/Playground/eslint-issue-9746/node_modules/eslint/lib/util/node_modules',
  '/home/nicolas/Playground/eslint-issue-9746/node_modules/eslint/lib/node_modules',
  '/home/nicolas/Playground/eslint-issue-9746/node_modules/eslint/node_modules',
  '/home/nicolas/Playground/eslint-issue-9746/node_modules',
  '/home/nicolas/Playground/node_modules',
  '/home/nicolas/node_modules',
  '/home/node_modules',
  '/node_modules',
  '/home/nicolas/.node_modules',
  '/home/nicolas/.node_libraries',
  '/usr/lib/node',
  '/home/nicolas/Playground/eslint-issue-9746/packages/eslint-example/node_modules',
  '/home/nicolas/Playground/eslint-issue-9746/packages/node_modules' ]

So basically it adds two new paths where the module could be found:

  '/home/nicolas/Playground/eslint-issue-9746/packages/eslint-example/node_modules',
  '/home/nicolas/Playground/eslint-issue-9746/packages/node_modules'

The module in this case is in /home/nicolas/Playground/eslint-issue-9746/packages/eslint-example/node_modules, which wasn't searched before because eslint is here /home/nicolas/Playground/eslint-issue-9746/node_modules/eslint

[not-an-aardvark]

Thanks, that description is useful. How does ESLint determine that it should search in the packages/eslint-example directory in this case? It's not clear to me where that path came from.

[ngotchac]

So the idea is that the Module resolver gets a extraLookup argument, which is an extra path where to look for a module.
These are extracts from the code, that would explain it better than I would. All of this is in /home/nicolas/Playground/eslint-issue-9746/node_modules/eslint/lib/config/config-file.js

extraLookup = getLookupPath(relativeTo);

function getLookupPath(configFilePath) {
    const basedir = getBaseDir(configFilePath);
    return path.join(basedir, "node_modules");
}

In my example, relativeTo === "/home/nicolas/Playground/eslint-issue-9746/packages/eslint-example"

Currently, getBaseDir(configFilePath) wrongly returns /home/nicolas/Playground/eslint-issue-9746/node_modules, thus getLookupPath(configFilePath) returns /home/nicolas/Playground/eslint-issue-9746/node_modules/node_modules.

With this PR, getBaseDir(configFilePath) returns /home/nicolas/Playground/eslint-issue-9746/packages/eslint-example, so getLookupPath(configFilePath) returns /home/nicolas/Playground/eslint-issue-9746/packages/eslint-example/node_modules.

The getBaseDir method looks like this:

function getBaseDir(configFilePath) {
    let projectPath = ...
    ...
}

Currently, the projectPath resolves to /home/nicolas/Playground/eslint-issue-9746/node_modules while it should (and that's what this PR does) resolves to /home/nicolas/Playground/eslint-issue-9746

So I guess to sum-up, I guess one can say that it mainly fixes the projectPath variable in getBaseDir. It actually also adds new paths to look for from this extraLookup path.

[ngotchac]

Just as a side note, I didn't quite get how projectPath = path.resolve(__dirname, "../../../") here actually makes sense, as I suspect it should actually be one folder up, so projectPath = path.resolve(__dirname, "../../../../"), but this would break tests so I worked around it.

[not-an-aardvark]

Sorry, I should have been more clear: I'm asking about the proposed design of this new feature, not the implementation. We can discuss the implementation in #9896 when/if the design is accepted. For the purposes of the discussion in this issue, though, it's not important to me how the change is implemented (including methods like getBaseDir) -- I'm just trying to understand what the external behavior would look like to a user who isn't familiar with ESLint internals.

To clarify the question from #9897 (comment): Why does ESLint search for modules from the packages/eslint-example directory, as opposed to some other arbitrary directory like foo/bar? (Is there a config file in packages/eslint-example that ESLint is being told about? Is the file which is getting linted located in packages/eslint-example?)

[ngotchac]

Sorry for the confusion.
So yes, the files to be linted are in packages/eslint-example, but all the configuration is in packages/eslint-config-example.

Basically, I guess the PR could be divided into 2 parts:

1. In some cases, getBaseDir returns a wrong value, a node_modules folder, which is not the base directory
2. Some folders are not looked into by the Module resolver, as it only looks at the top folder passed by the extraLookup variable, and not it's parents directories.

Is that a bit better?

[not-an-aardvark]

That does help. However, I am still confused about how this affects user-facing behavior in general, because you're describing it in terms of implementation details such as getBaseDir and extraLookup, which users wouldn't be aware of.

Again, this issue is intended to discuss the external behavior that users would see, not the implementation. If we were to put an entry in the changelog that says "Updated the return value of getBaseDir", this would not be useful to users, because getBaseDir is an implementation detail that users generally don't know about. On the other hand, if we were to put an entry in the changelog that says "Update config resolution to also search for shareable configs from the location of the files being linted", then this would be more useful to users because they would understand how this change affects them.

I am looking for the second kind of description for this change. You're proposing a change to user-facing behavior, and it might be worthwhile, but I can't tell because I'm not sure what the proposed change actually does. I'm asking for a general description of how the proposed change affects users, without referencing implementation details such as getBaseDir.

[ngotchac]

Ok yeah I see. So the idea here is that the eslint config and plugins were all in a module linked by npm via symlinks, the two modules being adjacents.
So maybe something around Enable (adjacent) sub-npm-modules to share ESLint config and plugins ? Although that might be a bit specific to Lerna, which builds this kind of structure.
Again, I'm not sure there is a right way of explaining the issue, because it's mostly specific modifications of the core code. Sorry if it's getting a bit lenghty...

[not-an-aardvark]

Again, I'm not sure there is a right way of explaining the issue, because it's mostly specific modifications of the core code.

I think you're thinking about this from the wrong direction. I want us to come up with a workable and coherent design for a change first, and then we can use that to create an implementation. The user-facing impact of the change is much more important than how it's implemented. If we just implement a change and then try to explain how it works afterwards, then there is no guarantee that the user-facing behavior is coherent or useful. (This is why we ask people to create an issue for core modifications -- so that we can discuss the design separately from the implementation.)

(Sorry about all the back-and-forth -- I don't mean to sound hostile, and I really appreciate your contributions. There are a lot of people relying on the existing config-resolution behavior, and I just want to make sure that if we make a change, the new behavior is well-reasoned and doesn't violate any existing user expectations.)

[not-an-aardvark]

In the example from #9746, lerna sets up a directory structure like this:

.
├── node_modules/
|   ├── eslint/
└── packages/
    ├── eslint-config-example/
    |   ├── index.js
    └── project-folder/
        ├── .eslintrc.js
        └── node_modules/
            └── eslint-config-example (symlink to ./packages/eslint-config-example)

ESLint resolves shareable configs from the location of the eslint package itself. As a result, it doesn't find eslint-config-example because eslint-config-example is located in ./packages/eslint-config-example rather than ./node_modules/eslint-config-example.

This means any solution that makes this "just work" with lerna would need to provide some way for ESLint to know where should find eslint-config-example in the above case.

[ngotchac]

Yes, I totally see your point, and agree. However, I cannot wrap my head around finding a clear explanation of the issue.

Here how it went on my side:

1. I couldn't get my shareable config to work, as explained in npm 5 link / lerna resolve issue #9746
2. I was looking through ESLint codebase, and found this part : https://github.com/eslint/eslint/blob/master/lib/config/config-file.js#L317L341
3. I actually found what I think is a bug in this function as it does include the node_modules in the path (see the comment) for this specific example
4. Fixing this bug actually fixed the issue.

So again, not sure if I can think of a clear explanation...

[not-an-aardvark]

You're right, that is a bit strange. It looks like that behavior was introduced as part of a fix for #5164.

[not-an-aardvark]

Okay, so here's what I could find:

• I think the behavior implemented in 8e4c41a has had a bug since it was implemented.

• As a result, ESLint is not behaving as described config documentation. (I think I was incorrect before when I said it was working as intended.)

  • The documentation says that for config files in a project that directly depends on ESLint, references to shareable configs in an extends clause will be resolved from the location of that config file.
  • In fact, the current behavior is that shareable configs in extends clauses are always resolved from the location of eslint itself.
  • This does not apply when the config is in the node_modules folder -- in that case the shareable config is resolved from the location of the config that extends it. However, in most cases the difference doesn't matter because if the config is in node_modules, then it would be able to access the same configs as ESLint directly.
  • This also does not apply to relative paths in extends clauses, which are always resolved from the location of the config file.

• For entirely unrelated reasons, plugins are always resolved from the location of ESLint. If we changed the resolution behavior for shareable configs only, then it would be inconsistent with the behavior for shareable plugins.

  • Making plugins resolve relative to a config file is blocked by Support having plugins as dependencies in shareable config #3458, because plugins currently need to have globally-unique names.
  • In my opinion, this wouldn't necessarily be a blocker for fixing shareable configs first.

• Changing the current behavior for configs would be a breaking change for directory structures like this:

  .
  ├── .eslintrc.json
  ├── node_modules/
  |   ├── eslint/
  |   └── eslint-config-foo/
  └── lib/
      ├── node_modules/
      |   └── eslint-config-foo/
      └── config-extension.json
  

  This is doable but would need to be a major release.

[ngotchac]

I'm not sure I see the issue with the last directory structure example. How could this setup happen?

[not-an-aardvark]

It's unlikely to happen as a result of using npm directly, but it could appear when using a tool that arranges node_modules folders in various places (like lerna). The issue is that if config-extension.json references eslint-config-foo, it would be resolved as ./lib/node_modules/eslint-config-foo with this fix applied (rather than ./node_modules/eslint-config-foo with the current behavior), which could be an entirely different package (or an incompatible version of the same package).

[ngotchac]

But wouldn't it rightly make sense to take the eslint-config-foo the closest to the config-extension.json? I guess if a user installs the same module but different versions at the root of the project and in the config sub-directory, he would expect ESLint to choose the version of the config sub-folder.

[not-an-aardvark]

I agree that it probably makes the most sense to do that. I am clarifying that doing it this way would be a breaking change from the way that ESLint currently resolves that config.

[ngotchac]

Ok, right. So should this issue be closed in favour of a more general one? I think you would word it in a better way that I could...

[medikoo]

I just approached this issue. ESLint doesn't follow symlinks as expected.

e.g. error can be easily reproduced with following setup:

Listing of files in ~/projects/project-a

node_modules/eslint            (directory, working eslint installation)
node_modules/eslint-config-x   (symlink to `~/projects/eslint-config-x`)
.eslintrc.json                 (references: "extends": "x")

Listing of files in ~/projects/eslint-config-x

node_modules/eslint-config-y   (symlink to `~/projects/eslint-config-y`)
index.js                       (references ""extends": "y"(

Listing of files in ~/projects/eslint-config-y

index.js (something valid)

Having above, running npx eslint . in ~/projects/project-a/. will throw as follows:

Error: Cannot find module 'eslint-config-y'
Referenced from: ~/projects/eslint-config-x/index.js
Referenced from: ~/projects/project-a/.eslintrc.json

It would be way better if if instead of trying to use internal Node.js functions, eslint would rely on node-resolve or cjs-module/resolve.

[not-an-aardvark]

Closing this issue in favor of #10643, since that issue addresses the problem in a more general way and has some more detail on various possible solutions.