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

Prefer local eslint #115

Closed
lencioni opened this issue Jan 9, 2016 · 7 comments
Closed

Prefer local eslint #115

lencioni opened this issue Jan 9, 2016 · 7 comments
Assignees

Comments

@lencioni
Copy link
Collaborator

lencioni commented Jan 9, 2016

In #114 I improved some error reporting that happens when packages that are expected by the local project's .eslintrc file are not installed globally. It would be nice, however, if import-js preferred the locally installed eslint over the globally installed one, so it could just rely on these packages being installed locally.

Normally, to execute local node_modules binaries, I do something like $(npm bin)/executable since npm bin will output the path to the local node_modules/.bin directory.

@lencioni
Copy link
Collaborator Author

lencioni commented Jan 9, 2016

It needs to be more sophisticated than a simple

diff --git a/lib/import_js/importer.rb b/lib/import_js/importer.rb
index 6835299..8e6de17 100644
--- a/lib/import_js/importer.rb
+++ b/lib/import_js/importer.rb
@@ -97,6 +97,7 @@ module ImportJS
         --rule 'no-undef: 2'
         --rule 'no-unused-vars: [2, { "vars": "all", "args": "none" }]'
       ].join(' ')
+      command = "$(npm bin)/#{command} || #{command}"
       out, err = Open3.capture3(command,
                                 stdin_data: @editor.current_file_content)

because eslint will exit with an exit code of 1 if any errors are found while linting, which will cause the fallback eslint command to be run.

@trotzig
Copy link
Collaborator

trotzig commented Jan 9, 2016

Instead of doing this dynamically, I think it would make sense to bring back the config that allowed you to override it. I have a feeling that might be useful in other cases as well.

@lencioni
Copy link
Collaborator Author

lencioni commented Jan 9, 2016

Yeah, maybe it could just be a boolean for simplicity? Should it default to local or global installation?

@trotzig
Copy link
Collaborator

trotzig commented Jan 9, 2016

Default to global, but allow override to e.g. 'node_modules/.bin/eslint'. I think a boolean is too "handholdy", we might as well let people deal with it themselves.

@trotzig
Copy link
Collaborator

trotzig commented Jan 9, 2016

I think you are in a better situation to test this out. Would you mind taking it on?

@trotzig
Copy link
Collaborator

trotzig commented Jan 10, 2016

Actually, this should be rather simple. I can take it on later tonight as long as the stars align.

@lencioni
Copy link
Collaborator Author

I'll work on it now

@lencioni lencioni self-assigned this Jan 10, 2016
lencioni added a commit to lencioni/import-js that referenced this issue Jan 10, 2016
If there is a local .eslintrc file that includes dependencies on
eslint-* packages, such as configurations, plugins, or parsers, running
the globally installed eslint can fail if these packages are not also
globally installed. To make this smoother for teams, I am adding a
configuration option that allows people to configure the eslint
executable, which enables them to use the locally installed eslint,
which should be able to find the other dependencies for their project.

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

No branches or pull requests

2 participants