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

flycheck-eslint does not use version of eslint installed in JavaScript project #1087

Closed
cowboyd opened this issue Sep 12, 2016 · 13 comments
Closed

Comments

@cowboyd
Copy link

cowboyd commented Sep 12, 2016

When you're entered into a project that has a version of eslint pegged in its package.json, I'd expect it to use that version. Instead, it always opts for the global version, or remains completely disabled if there is not a global version installed.

$ which eslint                                                                                                                                              
eslint not found
$ node_modules/.bin/eslint --version                                                                                                                      
v3.5.0

Environment

Syntax checkers for buffer routes/application.js in js2-mode:

  javascript-eslint
    - predicate:          nil
    - executable:         Not found
    - configuration file: Not found

  javascript-jshint
    - predicate:          nil
    - executable:         Not found
    - configuration file: Not found

  javascript-gjslint
    - predicate:          nil
    - executable:         Not found
    - configuration file: Not found

  javascript-jscs
    - predicate:          nil
    - executable:         Not found
    - configuration file: Not found

  javascript-standard
    - predicate:  nil
    - executable: Not found

Flycheck Mode is enabled.  Use C-u C-c ! x to enable disabled
checkers.

--------------------

Flycheck version: 30snapshot (package: 20160912.814)
Emacs version:    24.4.1
System:           x86_64-apple-darwin14.0.0
Window system:    ns
@swsnr
Copy link
Contributor

swsnr commented Sep 12, 2016

@cowboyd Why do you expect that? This behaviour is nowhere documented and does not exist in Flycheck.

Flycheck does not take language-specific package managers into account. Given the sheer number of these that's absolutely out of scope.

However Flycheck lets you override the executable of a syntax checker. Type C-c ! e javascript-eslint to make eslint use a different executable. You can also change the executable from Emacs Lisp—I've got code in my Emacs configuration that picks eslint from node_modules. Feel free to copy it, or write your own functions for your needs.

I'm closing this issue as wontfix as it's out of scope.

@cowboyd
Copy link
Author

cowboyd commented Sep 13, 2016

@lunaryorn I don't expect it based off of any understanding of how flycheck is supposed to work (I've never looked at the code). Instead, I expect it based on it being the most intuitive experience to a user.

If that means packaging the code to do it in a separate package, that's fine too. Thanks for the snippet! In your setup, how do you wire it up so that lunaryorn-use-js-executables-from-node-modules gets called at the right time?

@aaronjensen
Copy link
Member

@lunaryorn fwiw, most people at my office and myself have similar snippets. Spacemacs has a similar snippet built-in. It's a much more convenient way to run node tools. What would you think about building this in to the checker? The basics could be reused across all node checkers.

@swsnr
Copy link
Contributor

swsnr commented Sep 13, 2016

@cowboyd I simply add it to flycheck-mode-hook 😎

@aaronjensen I'm not sure how this would be built into the syntax checker, but in any case it's been our policy not to include too much language-specific logic into Flycheck, but rather provide an infrastructure upon which additional packages can build for best integration with languages. Hence flycheck-haskell or flycheck-rust.

That's not a dogma, though, but I think it's a pragmatic approach. We simply couldn't handle language integration in Flycheck—there's too much languages, too many different ways to integrate them, etc, and we're already short on resources. If we did that for JS, we'd have to do it for Haskell, Python, Perl, Ruby, C/C++, etc. and all the other languages that we support, and that's just too much.

If someone would like to create and maintain a flycheck-javascript package that includes my code and probably more we'll happily provide support in Flycheck and space in our organisation, but as far as Flycheck itself is concerned it's just way out of scope. Our's is just the checker definitions, the interfaces and the infrastructure.

@aaronjensen
Copy link
Member

I'm not sure how this would be built into the syntax checker

:command would need to support functions or we'd need to actually have hooks in place.

Thinking outside of the box, another way would be to have :working-directory set to find the directory with node_modules and then include node_modules/.bin in PATH when executing eslint.

If we did that for JS, we'd have to do it for Haskell, Python, Perl, Ruby, C/C++, etc. and all the other languages that we support, and that's just too much.

I understand that, but it's important to have good/great defaults in my opinion. Relying on a globally installed package is easy, but it's an extra manual step for many projects, not to mention the fact that they have to also install any plugins they use and it gets tricky if different projects on their machine use different versions of things.

@Simplify
Copy link
Member

Did you try to create .dir-locals.el in your project directory?

((nil . ((eval . (progn
                   (add-to-list 'exec-path (concat (locate-dominating-file default-directory ".dir-locals.el") "node_modules/.bin/")))))))

This is what I use in all my projects. I wrote blog article about it 7 months ago and it's first result when you google for "emacs node_modules/.bin in your path".

When you start GNU Emacs first time after creating this file, Emacs will ask you do you trust this file. When you select y or yes (depending of your Emacs config), Emacs will remember signature of this setting and you can use use same file in different projects. many people underestimate power of .dir-locals.el. I use it all the times. For example in Ember.js projects you can disable ESLint (JSHint is default in Ember) and activate ember-mode.

@cpitclaudel
Copy link
Member

@Simplify wouldn't it be better to use expand-file-name there?

@Simplify
Copy link
Member

@cpitclaudel It makes more sense and uses default-directory as default. I'll try it tomorrow with some JS files in project sub-directories and update blog post if needed.

@swsnr
Copy link
Contributor

swsnr commented Sep 13, 2016

We already have hooks that let you preprocess :command before executing and change how Flycheck looks for executable files. The infrastructure is already there.

Someone just needs to build a package on top of this that provides integration with nodejs 😊

@arthurl
Copy link

arthurl commented Dec 10, 2016

@Simplify: I really liked your solution for it's simplicity, but wouldn't that mean that the path is added globally, persisting even after the buffer is closed?

For example, say I opened a JS file in project A, then opened and closed another file in project B just to peek at it (I do this all the time for code snippets), and then returned to the first file. Wouldn’t flycheck be using the path from project B at this time?

@Simplify
Copy link
Member

@arthurl I that case PATH of project A will be used before PATH of project B, and you may end up using eslint from project A in that file of project B. That is not a big deal if you are just copying code from project B. There is well a problem if you are working on two projects at the same time from same Emacs session. Personally, I have no problem with that, I'm using GNU Emacs only in terminal and have different terminal windows or tabs for different projects.

As Sebastian said, infrastructure is there for anyone who wants to build package/extension for Flycheck. I don't believe that executable lookup beyond PATH for node/js projects should be part of default Flycheck because there is a lot of work to build that and support to provide after. There are many different ways to install NodeJS (package from the site, homebrew, ...) and there are different node version managers out there (3 that I know of) that install nodejs in different places and different paths. Even if we add this to Flycheck I will expect a lot of tickets in the vain of "Why is Flycheck selecting executable that I don't want?".

@maio
Copy link
Contributor

maio commented Dec 16, 2016

https://github.com/codesuki/add-node-modules-path solves this problem.

@arthurl
Copy link

arthurl commented Dec 16, 2016

@Simplify I totally agree with the point of view that this shouldn't be part of flycheck itself. I'm just coming from the point of view of a relatively novice emacs user (who is already spoilt by all the great work done in things like intero or flycheck-haskell) looking for a solution.

@maio Thanks, that is exactly what I need!

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

7 participants