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 issues error when running with eslint #1350

Closed
zebralight opened this Issue Oct 26, 2017 · 19 comments

Comments

Projects
None yet
5 participants
@zebralight

zebralight commented Oct 26, 2017

  • What you did, and what you expected to happen instead
    Expected: I opened my javascript file and expected that I'd be able to work on the file with flycheck checking my syntax on the fly with no error. I setup a workaround such that I switch to js2-mode from the default js-mode and then I see linting activity. But

Actual: Got error message and there is no linting activity.
EDIT: I see linting activity in js2-mode but not in js-mode and even though I see linting activity in js2-mode, every time I save, the error message is displayed in my mini-buffer:

Suspicious state from syntax checker javascript-eslint: Flycheck checker javascript-eslint returned non-zero exit code 1, but its output contained no errors: <?xml version="1.0" encoding="utf-8"?><checkstyle version="4.3"><file name="/home/user/code/randomDataGen/randomDataGen.js"><error line="2" column="7" severity="warning" message="&apos;Promise&apos; is assigned a value but never used. (no-unused-vars)" source="eslint.rules.no-unused-vars" /><error line="3" column="7" severity="warning" message="&apos;rp&apos; is assigned a value but never used. (no-unused-vars)" source="eslint.rules.no-unused-vars" /><error line="16" column="1" severity="error" message="Expected indentation of 0 spaces but found 2. (indent)" source="eslint.rules.indent" /><error line="16" column="29" severity="warning" message="&apos;res&apos; is defined but never used. (no-unused-vars)" source="eslint.rules.no-unused-vars" /><error line="17" column="1" severity="error" message="Expected indentation of 2 spaces but found 4. (indent)" source="eslint.rules.indent" /><error line="18" column="1" severity="error" message="Expected indentation of 4 spaces but found 6. (indent)" source="eslint.rules.indent" /><error line="19" column="1" severity="error" message="Expected indentation of 2 spaces but found 4. (indent)" source="eslint.rules.indent" /><error line="20" column="1" severity="error" message="Expected indentation of 0 spaces but found 2. (indent)" source="eslint.rules.indent" /><error line="20" column="5" severity="error" message="Missing semicolon. (semi)" source="eslint.rules.semi" /><error line="23" column="1" severity="error" message="Expected indentation of 0 spaces but found 1. (indent)" source="eslint.rules.indent" /><error line="48" column="1" severity="error" message="Expected indentation of 2 spaces but found 3. (indent)" source="eslint.rules.indent" /><error line="48" column="12" severity="error" message="Missing semicolon. (semi)" source="eslint.rules.semi" /></file></checkstyle>
(node:11755) DeprecationWarning: [eslint] The 'ecmaFeatures' config file property is deprecated, and has no effect. (found in /home/user/code/randomDataGen/.eslintrc)

  • Whether and how you were able to [reproduce the issue in emacs -Q][emacsQ]: yes
  • Flycheck setup from M-x flycheck-verify-setup:
Syntax checkers for buffer randomDataGen.js in js-mode:

  javascript-eslint
    - may enable:  yes
    - executable:  Found at /usr/bin/eslint
    - config file: found

  javascript-jshint (disabled)
    - may enable:         Automatically disabled!
    - executable:         Not found
    - configuration file: Not found

  javascript-jscs (disabled)
    - may enable:         Automatically disabled!
    - executable:         Not found
    - configuration file: Not found

  javascript-standard
    - may enable: Automatically disabled!
    - executable: Not found

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

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

Flycheck version: 32snapshot (package: 20171022.709)
Emacs version:    25.3.1
System:           x86_64-pc-linux-gnu
Window system:    x

  • Arch Linux
  • Emacs version GNU Emacs 25.3.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.19) of 2017-09-16
  • Flycheck version (package: 20171022.709)

my .eslintrc file
eslint version: 4.8.0

{"extends": "airbnb"}
@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Oct 26, 2017

Thanks for the report. Can you show the contents of /home/user/code/randomDataGen/.eslintrc? Is it only that {"extends": …} statement?

@zebralight

This comment has been minimized.

zebralight commented Oct 26, 2017

thanks for responding. That is correct. It is only that extends object

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Oct 26, 2017

@Simplify

This comment has been minimized.

Member

Simplify commented Oct 26, 2017

(node:11755) DeprecationWarning: [eslint] The 'ecmaFeatures' config file property is deprecated, and has no effect. (found in /home/user/code/randomDataGen/.eslintrc)

The problem is exit code 1 that comes from xml parser. eslint will print deprecation warning before actual XML output. Right now flycheck does not distinguish strout and stderr so everything that we get from eslint we feed to XML parser. My understanding is that changing this will also have implications to other parsers in flycheck. See GH-1323.

Easy solution is to take airbrb config and paste it in your .eslintrc file and than remove ecmaFeatures from config. You can run eslint --print-config . to get resolved config without any extends.

You can also try to downgrade eslint to a version that does not have ecmaFeatures deprecated until that ruleset gets removed from airbrb eslint config.

You should let airbnb plugin maintainers know that ecmaFeatures is deprecated and should be removed.

Long term solution is to modify flycheck to parse JSON output from eslint instead of XML. That way we can use flycheck-parse-json to remove all non JSON data (all deprecation warnings).

@ljharb

This comment has been minimized.

ljharb commented Oct 26, 2017

Although the property has already been removed; deprecated properties should still work; until we upgrade to an eslint version that removes it, there's no "should" about it being removed.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Oct 26, 2017

@ljharb I think @Simplify meant to say that it should be removed to silence the warning. Or maybe I missed the point you were making?

@ljharb

This comment has been minimized.

ljharb commented Oct 26, 2017

It certainly can be removed to silence the warning (and was, in fact, removed 3 months ago; the OP is using an out-of-date version of the airbnb config); but that doesn't solve the root issue, which is that deprecation warnings are both expected and normal, and shouldn't disrupt anything.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Oct 26, 2017

which is that deprecation warnings are both expected and normal, and shouldn't disrupt anything.

Yup, we all agree on this :)

@zebralight

This comment has been minimized.

zebralight commented Oct 26, 2017

I ran npm i eslint -g again and that updated eslint to 4.9.0. I did a couple of other things and unfortunately, I didn't quite document what they were. I'll just chalk it up to following ljharb's advice to update my version of eslint. My .eslintrc is now working with simply just {"extend": "airbnb"} in it. Thanks everyone. Closing this issue now.

@zebralight zebralight closed this Oct 26, 2017

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Oct 26, 2017

I'm not sure this should be closed. We have a problem with eslint printing non-xml output to stderr in some cases, and I'd like us to fix that — either by convincing the eslint people to not do that, or by working around it somehow. Are we tracking this issue somewhere else? I know it has popped up before.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Oct 26, 2017

Are you thinking of #1323?

I agree we should improve the situation. In my opinion, if we ask for XML output, eslint should output everything as XML. That's how cargo works with the JSON output. But maybe they have good reasons to do otherwise.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Oct 26, 2017

I agree. I'll ask the eslint people.

@cpitclaudel

This comment has been minimized.

@zebralight

This comment has been minimized.

zebralight commented Oct 26, 2017

@cpitclaudel good point. Although the linter behaves as expected without the previous Suspicious state.... which was significantly more salient and annoying, I'm actually still getting a bunch of error messages in the background it appears:
Warning (flycheck): Syntax checker javascript-eslint reported too many errors (641) and is disabled.

While it seems that you've identified the source as being tied to eslint more than flycheck, I've reopened the issue anyway as you had advised.

@zebralight zebralight reopened this Oct 26, 2017

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Dec 4, 2017

So, according to the reply here:

https://gitter.im/eslint/eslint?at=59f34dd5d6c36fca31a404ea

The ESLint devs have no intention of writing XML to stderr, and tools that consume their output should separate stdout and stderr.

So we can either do that, or switch to JSON, where we already ignore non-JSON lines, as @Simplify suggested.

I think the second approach is the right one, since I suppose that on internal errors, eslint reports on stderr and returns non-zero. So if we ignore stderr, we won't have anything to display to the user on a suspicious checker state. If we parse the error using JSON, then the parser will ignore non-JSON lines, but they will still show up for the suspicious checker state report.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Dec 4, 2017

Agreed. Option 1 will probably be better in the long run, but option 2 is a good compromise.

@Simplify

This comment has been minimized.

Member

Simplify commented Dec 6, 2017

Ok, I'll create PR for option 2 in next few days.

@Simplify

This comment has been minimized.

Member

Simplify commented Dec 6, 2017

Ok, JSON formatter in ESLint prints too much...

Command (inside test/resources/languages/javascript):

$ eslint -f json syntax-error.js 

Output:

[{"filePath":"/Users/sasa/Projects/elisp/flycheck/test/resources/language/javascript/syntax-error.js",
   "messages":[
       {"ruleId":null,
        "fatal":true,
        "severity":2,
        "source":"if ( /* nothing here */ ) // comment",
        "message":"Parsing error: Unexpected token)", 
        "line":3,
        "column":25
      }],
  "errorCount":1,
  "warningCount":0,
  "fixableErrorCount":0,
  "fixableWarningCount":0,
  "source":"/** A bad if */\n\nif ( /* nothing here */ ) // comment\n"}]

Last "source" includes full content of scanned js file (only if there is warning or error detected)! However, scanning is fast, parser will scan full jquery-ui in 2-3 seconds on my laptop. No idea how long will it take to parse that JSON output in Emacs, but will know in next few days.

There are also some edge cases, for example if user opens minified js file. In that case whole js file is one line and not only that we will get full js file in last "source", but "source" for every warning/error in "messages" will include full js file content.

If parsing takes too long in Emacs, we can use some other formatter for example "compact".

Simplify added a commit that referenced this issue Dec 11, 2017

@Simplify

This comment has been minimized.

Member

Simplify commented Dec 11, 2017

PR is ready. Speed on large files is ok.

Simplify added a commit that referenced this issue Dec 13, 2017

Simplify added a commit that referenced this issue Dec 13, 2017

Simplify added a commit that referenced this issue Dec 13, 2017

Merge pull request #1372 from flycheck/GH-1350
GH-1350 - Change ESLint parsing from XML to JSON
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment