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

Extends resolver does not work correctly when .eslintrc and eslint are placed in different modules with npm@2 (or, not flat) #6338

Closed
roymiloh opened this issue Jun 7, 2016 · 7 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion question This issue asks a question about ESLint

Comments

@roymiloh
Copy link

roymiloh commented Jun 7, 2016

What version of ESLint are you using?
2.11.1

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:

// eslint-config-custom module
module.exports = {
  extends: 'xo',
  rules: {
    'no-bitwise': 2
  }
};
// .eslintrc inside the project
{
  "extends": "custom"
}

What did you do? Please include the actual source code causing the issue.
I have created a CLI that runs eslint and now using it in various projects. The actual eslint dependency is there, but the .eslintrc is inside the projects that use this CLI.
In addition, I have another module that inherits from xo configs and extends them. The structure looks like this:

my-repo
  .eslintrc
  /node_modules
    eslint-config-custom
      index.js
      /node_modules
        eslint-config-xo
    my-cli
      cli.js (here eslint runs)

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

Error: Cannot find module 'eslint-config-xo'
Referenced from: custom
Referenced from: my-repo/.eslintrc
    at Object.ModuleResolver.resolve (my-repo/node_modules/my-cli/node_modules/eslint/lib/util/module-resolver.js:78:19)
    at resolve (my-repo/node_modules/my-cli/node_modules/eslint/lib/config/config-file.js:473:33)
    at load (my-repo/node_modules/my-cli/node_modules/eslint/lib/config/config-file.js:490:24)
    at my-repo/node_modules/my-cli/node_modules/eslint/lib/config/config-file.js:386:36
    at Array.reduceRight (native)
    at applyExtends (my-repo/node_modules/my-cli/node_modules/eslint/lib/config/config-file.js:363:28)
    at load (my-repo/node_modules/my-cli/node_modules/eslint/lib/config/config-file.js:525:22)
    at my-repo/node_modules/my-cli/node_modules/eslint/lib/config/config-file.js:386:36
    at Array.reduceRight (native)
    at applyExtends (my-repo/node_modules/my-cli/node_modules/eslint/lib/config/config-file.js:363:28)

It's probably worth to mention that I'm using npm@2, so my dependencies (as you can see) are not flatten. It would work if I use npm@3 (or, say, npm dedupe) due to lookupPaths.

I have started to investigate that a little bit, and it looks like there's a problem with resolving the baseDir. projectPath is related to my CLI path my-repo/node_modules/my-cli/node_modules (it takes the place where eslint was started and go back ../../../), and configFilePath is my-repo/.eslintrc; then things are getting worse because pathIsInside(..) equals to false and projectPath is returned.

It resolves eslint-config-custom successfully as it's located inside my-repo/node_modules and it's a part of the lookupPaths.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jun 7, 2016
@ilyavolodin
Copy link
Member

The problem here is that dependencies are resolved by ESLint itself, not by shareable config. So in the case your shareable config extends something else, ESLint has to have a way to find that something else. Which means it has to be installed as peerDependancy of your config, not as a dependency.
@nzakas The change you did to resolve configs based on the position of the config file doesn't apply here, right? I can't seem to remember if it should apply to extends or not.

@ilyavolodin ilyavolodin added question This issue asks a question about ESLint and removed triage An ESLint team member will look at this issue soon labels Jun 9, 2016
@mysticatea
Copy link
Member

Weird.
We have a test for chaining "extends":

it("should load information from `extends` chain.", function() {
var config = ConfigFile.load(getFixturePath("extends-chain/.eslintrc.json"));
assert.deepEqual(config, {
env: {},
extends: "a",
globals: {},
parserOptions: {},
rules: {
a: 2, // from node_modules/eslint-config-a
b: 2, // from node_modules/eslint-config-a/node_modules/eslint-config-b
c: 2 // from node_modules/eslint-config-a/node_modules/eslint-config-b/node_modules/eslint-config-c
}
});
});

@roymiloh
Copy link
Author

roymiloh commented Jun 9, 2016

@mysticatea, it's not only about chaining but running eslint from a different place (say, in my case, node_modules/my_cli/node_modules/eslint) as well.

@ilyavolodin, peerDependencies will work for sure, but is this the solution you'd expect for (other possible solutions could be using npm 3, npm dedupe etc, but it would be nice if it works OOTB)?

@mysticatea
Copy link
Member

I got it.
If my memory is correct, we have kept old behavior for global installed ESLint. And ESLint compare between the location of ESLint and cwd in order to distinguish it's installed global or not.

@nzakas
Copy link
Member

nzakas commented Jun 13, 2016

We don't distinguish if ESLint is installed globally or not (there's no proven way to do that). IIRC, what we did was resolve from the config file if it wasn't in the home directory (~/.eslintrc) and use the original behavior when loading ~/.eslintrc.

What's the relationship between this issue and #6358?

@mysticatea
Copy link
Member

What's the relationship between this issue and #6358?

It was my misunderstanding.

kmdavis added a commit to handshake/gruntify-eslint that referenced this issue Jun 20, 2016
per eslint/eslint#6338 ESLint needs to be a peerDependency, in order for `extends` clauses to work properly.
@nzakas
Copy link
Member

nzakas commented Sep 1, 2016

Unfortunately, it looks like consensus couldn't be reached on this issue and so I'm closing it. While we wish we'd be able to accommodate everyone's requests, we do need to prioritize. We've found that issues failing to reach consensus after 21 days tend never to reach consensus, and as such, we close those issues. This doesn't mean the idea isn't interesting, just that it's not something the team can commit to.

@nzakas nzakas closed this as completed Sep 1, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion question This issue asks a question about ESLint
Projects
None yet
Development

No branches or pull requests

5 participants