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: Use cwd from executable location to fix nested projects #3222

Merged
merged 2 commits into from
Jul 8, 2020

Conversation

kevinoid
Copy link
Contributor

@kevinoid kevinoid commented Jul 2, 2020

Since #2937, ESLint is run from the nearest directory with node_modules. This does not work correctly for nested projects where the eslint and any required config/plugin packages are installed 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 SublimeLinter/SublimeLinter#1649 and NodeLinter#find_local_executable.

Thanks for considering,
Kevin

cc: @w0rp, @joeldodge79

kevinoid added 2 commits July 2, 2020 09:03
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]>
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]>
@joeldodge79
Copy link

this works perfectly for my setup in #3143 (comment)

thanks a ton for working on this!

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

This is a good solution. Thanks for this.

@w0rp w0rp merged commit 106c276 into dense-analysis:master Jul 8, 2020
@w0rp
Copy link
Member

w0rp commented Jul 8, 2020

Cheers! 🍻

w0rp pushed a commit that referenced this pull request 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]>
@kevinoid
Copy link
Contributor Author

kevinoid commented Jul 8, 2020

Thanks for reviewing and merging @w0rp! 🥂

@jakebrinkmann
Copy link

I am having a problem which is related to this MR. Should I open a new issue, or ask the questions here?

In summary, running eslint from my source code directory is desired, but this MR introduced changing directory to the location of eslint (thereby using the wrong configuration, instead of my projects eslint configuration)

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

Successfully merging this pull request may close these issues.

4 participants