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

Fix: wrong baseDir (fixes #6450) #6457

Merged
merged 1 commit into from Jun 20, 2016
Merged

Fix: wrong baseDir (fixes #6450) #6457

merged 1 commit into from Jun 20, 2016

Conversation

mysticatea
Copy link
Member

Fixes #6450

I'm not sure this fix is correct or not. I want to get any insights about getBaseDir.

And actually, this new test does not make sense. The test succeeds as well before this fix. The issue's error seems to be appeared only if eslint is installed to another package. I'm not sure how to write tests for this fix...

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @nzakas, @SpainTrain and @MethodGrab to be potential reviewers

@@ -315,6 +315,10 @@ function getBaseDir(configFilePath) {
// calculates the path of the project including ESLint as dependency
var projectPath = path.resolve(__dirname, "../../../");

if (path.basename(projectPath) === "node_modules") {
projectPath = path.dirname(projectPath);
Copy link
Member

Choose a reason for hiding this comment

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

Yikes! This seems like a fragile test, there has to be something else going on.

@eslintbot
Copy link

LGTM

@mysticatea
Copy link
Member Author

I modified it, just add ../.

Before

load( C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\.eslintrc.json undefined undefined )
    resolvedPath: { filePath: 'C:\\Users\\t-nagashima.AD\\Documents\\GitHub\\sandbox\\.eslintrc.json' }
    dirname: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox
    basedir: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules
    lookupPath: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules\node_modules

After

load( C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\.eslintrc.json undefined undefined )
    resolvedPath: { filePath: 'C:\\Users\\t-nagashima.AD\\Documents\\GitHub\\sandbox\\.eslintrc.json' }
    dirname: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox
    basedir: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox
    lookupPath: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules

@SimenB
Copy link
Contributor

SimenB commented Jun 18, 2016

This fixes the problem for us (using extends: './index.js')

@ilyavolodin
Copy link
Member

That looks much better then the check for node_modules.

@nzakas
Copy link
Member

nzakas commented Jun 18, 2016

I think we are heading in the wrong direction here. Adding ../ also seems very hacky to me because this worked fine before #6359 was merged. I think the correct path forward is to revert #6359 and try to solve #6358 a different way.

@@ -313,7 +313,7 @@ function write(config, filePath) {
function getBaseDir(configFilePath) {

// calculates the path of the project including ESLint as dependency
var projectPath = path.resolve(__dirname, "../../../");
var projectPath = path.resolve(__dirname, "../../../../");
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense to me. If this file hasn't moved, then why is this change necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's wrong.
Do you think this basedir and lookupPath are correct?

load( C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\.eslintrc.json undefined undefined )
    resolvedPath: { filePath: 'C:\\Users\\t-nagashima.AD\\Documents\\GitHub\\sandbox\\.eslintrc.json' }
    dirname: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox
    basedir: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules
    lookupPath: C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules\node_modules

I think the intention of projectPath is the root directory of a project which depends on ESLint. But it's not so currently. It's the node_modules directory of a project which depends on ESLint.
Then basedir and lookupPath are calculated from projectPath.
Then the problematic relativeTo is the basedir.
Then it went to wrong.

Copy link
Member

Choose a reason for hiding this comment

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

@mysticatea You might be right, in this case basedir doesn't look right. But with your fix, globally installed ESLint is failing even worse. It's now two directories off from where it should be looking (before this fix, it was one dir off).

Copy link
Member Author

Choose a reason for hiding this comment

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

@ilyavolodin I confirmed the problem for globally installed ESLint.
So.... to fix this route cause, do we needs === "node_modules" check as same as the first way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm...
basedir is not the location of .eslintrc file for globally installed ESLint...

Copy link
Member Author

Choose a reason for hiding this comment

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

I got it.
We cannot use relativeTo to resolve relative paths in .eslintrc files.
So #6468 is the correct way to fix this problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering that this parser path is working fine with this wrong basedir and lookupPath?

Copy link
Member Author

Choose a reason for hiding this comment

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

I confirmed the parser path is not working (same as extends of v2.13.0).
We cannot use the basedir in order to resolve relative paths.

@nzakas
Copy link
Member

nzakas commented Jun 18, 2016

It also looks like the test isn't reproducing the bug that's being reported.

@nzakas
Copy link
Member

nzakas commented Jun 18, 2016

I think I figured it out here: #6468

@mysticatea
Copy link
Member Author

It also looks like the test isn't reproducing the bug that's being reported.

Please see my first comment.

And actually, this new test does not make sense. The test succeeds as well before this fix. The issue's error seems to be appeared only if eslint is installed to another package. I'm not sure how to write tests for this fix...

@eslintbot
Copy link

LGTM

1 similar comment
@eslintbot
Copy link

LGTM

@mysticatea
Copy link
Member Author

mysticatea commented Jun 20, 2016

I reverted the change of projectPath.
The value of getBaseDir() is used for only resolving module path, the resolving logic searches modules in ancestor directories also, so it's no problem even if it's C:\Users\t-nagashima.AD\Documents\GitHub\sandbox\node_modules\node_modules.

Then, I found that this parser path is not working as same as extends of v2.13.0.
I think the root cause is that we are using the value of getBaseDir() in order to resolve relative paths.
It's not the directory of the current file.

So I replaced basedir of both parser and extends by dirname.
dirname is the directory of the current file.

@eslintbot
Copy link

LGTM

@mysticatea
Copy link
Member Author

mysticatea commented Jun 20, 2016

I added some tests.

basedir is changed by whether .eslintrc exists in projectPath or not.
tests/fixtures directory exists in projectPath.
I added some tests which are executed outside of projectPath.
Those tests were failed before this fix.

@nzakas
Copy link
Member

nzakas commented Jun 20, 2016

Ah! This looks much better. LGTM

@nzakas nzakas merged commit 434de7f into master Jun 20, 2016
@mysticatea mysticatea deleted the core/fix-relateveto branch June 21, 2016 19:14
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression from 2.13.0 release - relative requires in extend
7 participants