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

Regression from 2.13.0 release - relative requires in extend #6450

Closed
hzoo opened this issue Jun 17, 2016 · 20 comments · Fixed by #6457
Closed

Regression from 2.13.0 release - relative requires in extend #6450

hzoo opened this issue Jun 17, 2016 · 20 comments · Fixed by #6457
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features

Comments

@hzoo
Copy link
Member

hzoo commented Jun 17, 2016

Ref #6359 I think, @mysticatea

What version of ESLint are you using?

2.13.0 (just released)

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

default

Please show your full configuration:

{
  "extends": "./node_modules/eslint-preset-behance/.eslintrc"
}

What did you do? Please include the actual source code causing the issue.

run eslint app_folder/

Updated to 2.13.0 in CI

What did you expect to happen?

No issues

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

Cannot read config file: app/node_modules/node_modules/eslint-preset-behance/.eslintrc
Error: ENOENT: no such file or directory, open 'app/node_modules/node_modules/eslint-preset-behance/.eslintrc'

It's checking node_modules/node_modules for some reason

Workaround I guess is that we don't have to use ./node_modules/

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jun 17, 2016
@Mardak
Copy link
Contributor

Mardak commented Jun 17, 2016

This seems to also result in going up too many parent directories: { "extends": "../../../test/.eslintrc" }

Results in:

Cannot read config file: /Users/test/.eslintrc
Referenced from: /Users/Mardak/foo/bar/baz/test/.eslintrc

Before it would extend /Users/Mardak/foo/test/.eslintrc

@paglias
Copy link

paglias commented Jun 17, 2016

Same here. It broke all our builds

@ilyavolodin ilyavolodin added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jun 17, 2016
@ilyavolodin
Copy link
Member

Issue was introduced by #6359 fix.

@hzoo hzoo changed the title Regression from https://github.com/eslint/eslint/pull/6359 Regression from 2.13.0 release - relative requires in extend Jun 17, 2016
@hzoo
Copy link
Member Author

hzoo commented Jun 17, 2016

for the change: path.join(relativeTo || path.dirname(filePath), parentPath)

parentPath: ./node_modules/eslint-preset-behance/.eslintrc
relativeTo: app/node_modules
path.dirname(filePath): app

path.join(path.dirname(filePath), parentPath)):
app/node_modules/eslint-preset-behance/.eslintrc

path.join(relativeTo, parentPath)):
app/node_modules/node_modules/eslint-preset-behance/.eslintrc

And it looks like the test wasn't testing the change in applyExtends but the input/output of ConfigFile.load

@apazzolini
Copy link

apazzolini commented Jun 17, 2016

This is an issue for me as well. In my instance, this error appears to be related to using eslint as a non-global package and running it with ./node_modules/.bin/eslint

@nzakas
Copy link
Member

nzakas commented Jun 17, 2016

Let's see what @mysticatea thinks. He should be around soonish and then we can decide whether there's an easy fix or not.

@mysticatea
Copy link
Member

I apologize for this.
I will investigate this.
I think relativeTo should be the directory that .eslintrc exists.

@mysticatea mysticatea self-assigned this Jun 17, 2016
@ilyavolodin
Copy link
Member

@mysticatea That's what relativeTo should be, but it doesn't look like it is in this case, for some reason. Might be a deeper bug in relativeTo itself (which would affect a lot of other things).

@mysticatea
Copy link
Member

Mmm, weird, we also have relative path in our setting and it's working: https://github.com/eslint/eslint/blob/414206cdec53f4967f56ee23ac0c1eb45b31846a/.eslintrc.yml

@mysticatea
Copy link
Member

That relativeTo seems to be

return path.join(projectPath);

@mysticatea
Copy link
Member

load( C:\Users\starc\Documents\GitHub\eslint\.eslintrc.yml undefined undefined )
    resolvedPath: { filePath: 'C:\\Users\\starc\\Documents\\GitHub\\eslint\\.eslintrc.yml' }
    dirname: C:\Users\starc\Documents\GitHub\eslint
    basedir: C:\Users\starc\Documents\GitHub\eslint\node_modules
    lookupPath: C:\Users\starc\Documents\GitHub\eslint\node_modules\node_modules

getBaseDir seems wrong, but if I fixed getBaseDir, I get failures of some tests...

mysticatea added a commit that referenced this issue Jun 17, 2016
@mysticatea
Copy link
Member

Fixed: 4c80417#diff-3ca2712fe1d95b19637e57eb1a8b3a5bR318

But I'm not sure this is a correct way or not...

@nzakas
Copy link
Member

nzakas commented Jun 18, 2016

I don't think special-casing node_modules is the right approach.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 18, 2016

Is there a deadline at which point the change that broke this will be reverted, absent a fix?

@ilyavolodin
Copy link
Member

ilyavolodin commented Jun 18, 2016

@ljharb We are usually try to release patches on the following Monday after the release that created an issue.

@mysticatea Another problem related to this change. You can't run global eslint at all anymore:

$ eslint .
Cannot read config file: C:\Users\ilya\AppData\Roaming\npm\node_modules\packages\eslint-config-eslint\default.yml
Error: ENOENT: no such file or directory, open 'C:\Users\ilya\AppData\Roaming\npm\node_modules\packages\eslint-config-eslint\default.yml'
Referenced from: c:\Users\ilya\Documents\github\eslint\.eslintrc.yml
Error: Cannot read config file: C:\Users\ilya\AppData\Roaming\npm\node_modules\packages\eslint-config-eslint\default.yml
Error: ENOENT: no such file or directory, open 'C:\Users\ilya\AppData\Roaming\npm\node_modules\packages\eslint-config-eslint\default.yml'
Referenced from: c:\Users\ilya\Documents\github\eslint\.eslintrc.yml
    at Error (native)
    at Object.fs.openSync (fs.js:549:18)
    at Object.fs.readFileSync (fs.js:397:15)
    at readFile (C:\Users\ilya\AppData\Roaming\npm\node_modules\eslint\lib\config\config-file.js:71:15)
    at loadYAMLConfigFile (C:\Users\ilya\AppData\Roaming\npm\node_modules\eslint\lib\config\config-file.js:102:30)
    at loadConfigFile (C:\Users\ilya\AppData\Roaming\npm\node_modules\eslint\lib\config\config-file.js:219:22)
    at load (C:\Users\ilya\AppData\Roaming\npm\node_modules\eslint\lib\config\config-file.js:500:18)
    at C:\Users\ilya\AppData\Roaming\npm\node_modules\eslint\lib\config\config-file.js:392:36
    at Array.reduceRight (native)
    at applyExtends (C:\Users\ilya\AppData\Roaming\npm\node_modules\eslint\lib\config\config-file.js:363:28)

It's looking for C:\Users\ilya\AppData\Roaming\npm\node_modules\packages\eslint-config-eslint\default.yml and correct path should be C:\Users\ilya\AppData\Roaming\npm\node_modules\eslint\packages\eslint-config-eslint\default.yml
So it's going one directory too far up.

@nzakas
Copy link
Member

nzakas commented Jun 18, 2016

I think I figured it out. See #6468

@travi
Copy link

travi commented Jun 20, 2016

I'm not sure this fully resolved the problem. I'm still seeing this error in a CI build that shows that it is pulling in the latest version, including this patch.

I started seeing this at the end of last week and have tracked it down to my use of grunt-eslint, which depends on "eslint": "^2.0.0". Therefore, my project does not directly define the eslint version to use since the grunt plugin depends on ^2.0.0

The piece that might be different in my case is that my extends definition is relative to the rule file rather than node_modules: https://github.com/travi/eslint-config-travi/blob/master/rules/tests/react.js#L2

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jun 20, 2016

(This fix did resolve my issue from #6458, fwiw, thanks!!!)

@nzakas
Copy link
Member

nzakas commented Jun 20, 2016

@travi please open a new issue and provide your details.

@travi
Copy link

travi commented Jun 21, 2016

sure thing: #6489. thank you for the help

crookedneighbor added a commit to HabitRPG/eslint-config that referenced this issue Jul 7, 2016
[Eslint 2.13.0 made some changes to how relative paths work when
extending](eslint/eslint#6450), which caused
any linting that used our config to fail.

By referencing the package name instead in our sub-configs, we resolve
this erroneous behavior.
@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
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants