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 doesn't work as a fixer when ESLint used with plugins and run in a parent directory of the project #3143

Closed
jgonera opened this issue Apr 29, 2020 · 12 comments
Labels

Comments

@jgonera
Copy link

jgonera commented Apr 29, 2020

Information

NVIM v0.4.3
Build type: Release

Operating System: macOS Catalina 10.15.2 (19C57)

What went wrong

When running ESLint as a fixer ALE is not cd-ing into the file's directory like it does when it runs ESLint as a linter:

# As a linter
(finished - exit code 1) ['/bin/zsh', '-c', 'cd ''/Users/juliusz/repos/ale-bug/project'' && ''/Users/juliusz/repos/ale-bug/project/node_modules/eslint/bin/eslint.js'' -f json --stdin --stdin-filename ''/Users/juliusz/repos/ale-bug/project/src/index.js'' < ''/var/folders/x6/bcf_tsv923nbj4mxn67vhwbh0000gn/T/nvimbxfwpz/7/index.js''']

# As a fixer
(finished - exit code 2) ['/bin/zsh', '-c', '''/Users/juliusz/repos/ale-bug/project/node_modules/eslint/bin/eslint.js'' --stdin-filename ''/Users/juliusz/repos/ale-bug/project/src/index.js'' --stdin --fix-dry-run --format=json < ''/var/folders/x6/bcf_tsv923nbj4mxn67vhwbh0000gn/T/nvimbxfwpz/5/index.js''']

In such case ESLint is run in Vim's :pwd, in my case /Users/juliusz/repos/ale-bug, while it should run in /Users/juliusz/repos/ale-bug/project which is where .eslintrc.js and node_modules are. This results in ESLint not being able to find the plugins and exiting with status 2. Doing :cd project makes ESLint work as a fixer.

If I try running ESLint manually while in /Users/juliusz/repos/ale-bug it outputs this (this output doesn't show up in :ALEInfo since it's stderr):

$ /bin/zsh -c '/Users/juliusz/repos/ale-bug/project/node_modules/eslint/bin/eslint.js --stdin-filename /Users/juliusz/repos/ale-bug/project/src/index.js --stdin --fix-dry-run --format=json < /Users/juliusz/repos/ale-bug/project/src/index.js'

Oops! Something went wrong! :(

ESLint: 6.8.0.

ESLint couldn't find the plugin "eslint-plugin-node".

(The package "eslint-plugin-node" was not found when loaded as a Node module from the directory "/Users/juliusz/repos/ale-bug".)

It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:

    npm install eslint-plugin-node@latest --save-dev

The plugin "eslint-plugin-node" was referenced from the config file in "project/.eslintrc.js".

If you still can't figure out the problem, please stop by https://gitter.im/eslint/eslint to chat with the team.

Reproducing the bug

Assuming the following Vim config:

let g:ale_fixers = {
  \ 'javascript': ['eslint'],
\}
  1. git clone https://github.com/jgonera/ale-bug.git
  2. cd ale-bug
  3. (n)vim project/src/index.js

ALE will correctly highlight 123 === NUMBER as a linting error for yoda ESLint rule, but :ALEFix will not fix it. If you run :cd project and then :ALEFix, then everything will work.

@jgonera
Copy link
Author

jgonera commented Apr 29, 2020

@w0rp follow-up for #978 (comment)

I made a repo with a minimal repro case. It seems the issue only occurs when there are ESLint plugins in use (ESLint can find the config but not the plugins). cd-ing to the directory of the file (like when linting) should hopefully fix it anyway.

@joeldodge79
Copy link

joeldodge79 commented Jul 1, 2020

perhaps related:

I'm working in a monorepo that uses Yarn Workspaces where the top level has all the eslint stuff (top level package.json with eslintConfig, node_modules/eslint). I'm trying out SpaceVim/nvim using ALE instead of the default Neomake.

It seems like ALE is cd'ing into the child package but that's no good in my case: it should stay put at the root level.

Monorepo structure
/usr/local/home/joeldodge/sdk-codegen is the root (where all the eslint stuff lives)
/usr/local/home/joeldodge/sdk-codegen/packages/sdk-codegen is the sdk-codegen package

nvim :pwd => /usr/local/home/joeldodge/sdk-codegen

(executable check - success) /usr/local/home/joeldodge/sdk-codegen/node_modules/eslint/bin/eslint.js
(finished - exit code 2) ['/usr/bin/zsh', '-c', 'cd ''/usr/local/home/joeldodge/sdk-codegen/packages/sdk-codegen'' && ''/usr/local/home/joeldodge/sdk-codegen/node_modules/eslint/bin/eslint.js'' -f json --stdin --stdin-filename ''/usr/local/home/joeldodge/sdk-codegen/packages/sdk-codegen/src/python.gen.ts'' < ''/tmp/nvimeRpRMu/2/python.gen.ts''']
<<<NO OUTPUT RETURNED>>>

running manually from sdk-codegen/ the command works. Running from sdk-codgen/packages/sdk-codgen/ fails with this stderr (would be helpful to see this in ALEInfo):

joeldodge@joeldodge ~/sdk-codegen
❯ pwd
/usr/local/home/joeldodge/sdk-codegen

joeldodge@joeldodge ~/sdk-codegen
❯ /usr/bin/zsh -c '/usr/local/home/joeldodge/sdk-codegen/node_modules/eslint/bin/eslint.js --stdin-filename /usr/local/home/joeldodge/sdk-codegen/packages/sdk-codegen/src/python.gen.ts --stdin --fix-dry-run --format=json < /usr/local/home/joeldodge/sdk-codegen/packages/sdk-codegen/src/python.gen.ts'
Error: Error while loading rule 'header/header': ENOENT: no such file or directory, open 'config/license-header.js'
Occurred while linting /usr/local/home/joeldodge/sdk-codegen/packages/sdk-codegen/src/python.gen.ts
    at Object.openSync (fs.js:440:3)
    at Object.readFileSync (fs.js:342:35)
    at Object.module.exports [as create] (/usr/local/home/joeldodge/sdk-codegen/node_modules/eslint-plugin-header/lib/rules/header.js:92:23)
    at createRuleListeners (/usr/local/home/joeldodge/sdk-codegen/node_modules/eslint/lib/linter/linter.js:746:21)
    at /usr/local/home/joeldodge/sdk-codegen/node_modules/eslint/lib/linter/linter.js:916:31
    at Array.forEach (<anonymous>)
    at runRules (/usr/local/home/joeldodge/sdk-codegen/node_modules/eslint/lib/linter/linter.js:861:34)
    at Linter._verifyWithoutProcessors (/usr/local/home/joeldodge/sdk-codegen/node_modules/eslint/lib/linter/linter.js:1157:31)
    at Linter._verifyWithConfigArray (/usr/local/home/joeldodge/sdk-codegen/node_modules/eslint/lib/linter/linter.js:1255:21)
    at Linter.verify (/usr/local/home/joeldodge/sdk-codegen/node_modules/eslint/lib/linter/linter.js:1210:25)

@joeldodge79
Copy link

joeldodge79 commented Jul 1, 2020

ah, reading the source code it looks like the logic is find the node_modules/ dir closest to the file and cd to that parent dir. I do have a sdk-codegen/packages/sdk-codegen/node_modules/ dir - it just doesn't have the eslint stuff, that's in the root node_modules/

and now I'm onto the trail of #2937 and #3096

@w0rp
Copy link
Member

w0rp commented Jul 1, 2020

I really don't like the change ESLint made to how it deals with project roots. It seems like every project that runs ESLInt will have to separately implement its own logic to discover where the ESLint configuration file is to know where to run ESLint from. That seems like a step backwards to me. SublimeLinter had the same issue: https://github.com/SublimeLinter/SublimeLinter/pull/1649/files

My suggestion is to always create an ESLint configuration file in the same directory as your node_modules directories, which for monorepos could be an almost empty file that loads the rest of the configuration from a root configuration file with root: true. That should always work.

@kevinoid
Copy link
Contributor

kevinoid commented Jul 1, 2020

I really don't like the change ESLint made to how it deals with project roots.

I agree. Good find on the SublimeLinter issue.

I also agree that current logic of running from the nearest directory with node_modules should be improved. Is there agreement on how ale#handlers#eslint#GetCdString should work? If I understand both your suggestions:

  1. Run ESLint from project root dir where possible #2937 (review) - Run from the directory where package.json (or node_modules?) contains eslint.
  2. ESLint doesn't work as a fixer when ESLint used with plugins and run in a parent directory of the project #3143 (comment) - Run from the directory where .eslintrc* has root: true (or eslintConfig in package.json has root: true?)

Or perhaps some combination of the above with fallback?

@w0rp
Copy link
Member

w0rp commented Jul 1, 2020

I've merged that pull request for the fixers now to fix #3094.

If anyone knows a way to fix this issue without potentially breaking other things, tag me in a pull request, and someone can look at it eventually. What you can do now is create ESLint configuration files in every directory that has node_modules, and your can put your settings to apply across all of your monorepos in a parent directory with root: true.

@joeldodge79
Copy link

ok, I figured out a solution for my case that does not require any further changes to this code.

The issue is that I'm using the eslint-plugin-header and I have this under my "eslintConfig" section:

"rules": {
  "header/header": [
    2,
    "config/license-header.js"
  ],

the documentation explicitly says:

Due to limitations in eslint plugins, the file is read relative to the working directory that eslint is executed in. If you run eslint from elsewhere in your tree then the header file will not be found.

I found I was able to add a .eslintrc into packages/sdk-codegen/ and specify

{
  "rules": {
    "header/header": [
      2,
      "../../config/license-header.js"
    ]
  }
}

to override the parent config but inherit the rest. A little annoying but maybe better than further complicating the GetCdString logic? Although there may be other cases out there that could benefit from it.

Interestingly modifying packages/sdk-codegen/package.json with an overriding "eslintConfig" section didn't seem to work.

@jgonera
Copy link
Author

jgonera commented Jul 1, 2020

Thank you for the PR for #3094 @w0rp! It seems to me that it actually fixes my issue too so I'd consider this bug a duplicate of #3094 unless there's a corner case that I'm missing.

@joeldodge79
Copy link

joeldodge79 commented Jul 1, 2020

@kevinoid I'm back 🤦

my team is mostly on VSCode - I'm the outlier. installing the eslint plugin for VSCode just works out of the box for our current setup. The team is not hot on my adding a duplicate .eslintrc into every package to support my nvim quirks. It gets worse: we use lint-staged which, when run from the root bombs out with the config in the child package! I figured out a work around with this husky/lerna setup but that's gonna just add more copy/paste duplication in the child packages.

Edit: some are on Intellij which also "just works" as-is...

so... it would be awesome, if you have the time, to implement that more subtle GetCdString logic...

kevinoid added a commit to kevinoid/ale that referenced this issue Jul 2, 2020
Using the nearest directory with node_modules does not work correctly
for nested projects where the eslint dependencies are in the outer
project.  For example:
dense-analysis#3143 (comment)

Adopt the behavior of SublimeLinter, which runs from project_root
determined by the presence of the eslint executable in node_modules/.bin
(or eslint in dependencies/devDependencies of package.json, which we can
add later as necessary).  See [NodeLinter#find_local_executable].

[NodeLinter#find_local_executable]: https://github.com/SublimeLinter/SublimeLinter/blob/056e6f6/lint/base_linter/node_linter.py#L109

Signed-off-by: Kevin Locke <[email protected]>
@kevinoid
Copy link
Contributor

kevinoid commented Jul 2, 2020

Thanks for the updates @joeldodge79. Taking a closer look, I think the original issue here and in #2997 can be closed as fixed by #3096. The issue described in #3143 (comment) is different (it's not fixer-specific and is due to cd to wrong directory for nested projects rather than no cd at all).

I also took a closer look at how SublimeLinter works, and I like the idea of choosing the directory where eslint is installed as the project root for eslint. I've created #3222 to implement that behavior. You you could test the PR to confirm it fixes your issue, I'd appreciate it.

w0rp pushed a commit that referenced this issue Jul 8, 2020
* Split FindNearestExecutable from FindExecutable

The path searching in ale#node#FindExecutable() will be useful for
eslint.  Refactor it into a separate function so it can be used without
regard for the state of the _use_global and _executable variables.

Signed-off-by: Kevin Locke <[email protected]>

* eslint: Set project root from local executable

Using the nearest directory with node_modules does not work correctly
for nested projects where the eslint dependencies are in the outer
project.  For example:
#3143 (comment)

Adopt the behavior of SublimeLinter, which runs from project_root
determined by the presence of the eslint executable in node_modules/.bin
(or eslint in dependencies/devDependencies of package.json, which we can
add later as necessary).  See [NodeLinter#find_local_executable].

[NodeLinter#find_local_executable]: https://github.com/SublimeLinter/SublimeLinter/blob/056e6f6/lint/base_linter/node_linter.py#L109

Signed-off-by: Kevin Locke <[email protected]>
@w0rp
Copy link
Member

w0rp commented Jul 8, 2020

I've merged #3222 now, so that should fix this.

@w0rp w0rp closed this as completed Jul 8, 2020
w0rp pushed a commit that referenced this issue Jul 8, 2020
* Split FindNearestExecutable from FindExecutable

The path searching in ale#node#FindExecutable() will be useful for
eslint.  Refactor it into a separate function so it can be used without
regard for the state of the _use_global and _executable variables.

Signed-off-by: Kevin Locke <[email protected]>

* eslint: Set project root from local executable

Using the nearest directory with node_modules does not work correctly
for nested projects where the eslint dependencies are in the outer
project.  For example:
#3143 (comment)

Adopt the behavior of SublimeLinter, which runs from project_root
determined by the presence of the eslint executable in node_modules/.bin
(or eslint in dependencies/devDependencies of package.json, which we can
add later as necessary).  See [NodeLinter#find_local_executable].

[NodeLinter#find_local_executable]: https://github.com/SublimeLinter/SublimeLinter/blob/056e6f6/lint/base_linter/node_linter.py#L109

Signed-off-by: Kevin Locke <[email protected]>
@Wongmat
Copy link

Wongmat commented Aug 31, 2020

Thanks for the updates @joeldodge79. Taking a closer look, I think the original issue here and in #2997 can be closed as fixed by #3096. The issue described in #3143 (comment) is different (it's not fixer-specific and is due to cd to wrong directory for nested projects rather than no cd at all).

I also took a closer look at how SublimeLinter works, and I like the idea of choosing the directory where eslint is installed as the project root for eslint. I've created #3222 to implement that behavior. You you could test the PR to confirm it fixes your issue, I'd appreciate it.

Can confirm that #2997 does indeed seem to fixed. I'll go ahead and close it. Thanks a bunch! Hope I'll have some time soon to delve more into Vimscript so I can contribute more than just reporting bugs!

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

No branches or pull requests

5 participants