AtomLinter / linter-jshint

Atom linter plugin for JavaScript, using jshint.
147 stars 36 forks source link

Stopped working with v2.0.0 #214

Closed jeffschwartz closed 8 years ago

jeffschwartz commented 8 years ago

I'm running Atom v1.5.3 & after installing v2.0.0 jshint stopped working.

steelbrain commented 8 years ago

Can you please explain what you mean by stopped working? Also did you try restarting Atom? and can you try running jshint from the cli and show us what you see?

jeffschwartz commented 8 years ago

It isn't linting .js files - no errors and no warnings. It also doesn't lint when selecting lint from the command palate. Was working fine until the v2.0.0 update.

steelbrain commented 8 years ago

Does apm install linter-jshint@1 really fix this for you? If it does, can you show us the output of jshint . from cli?

jeffschwartz commented 8 years ago

I installed using Atom's update page and not the cl using apm. Are you requesting that I uninstall the plugin and then use the cli to install the plugin?

steelbrain commented 8 years ago

I'm asking you to confirm that there really is a bug with this update by temporarily rolling back to the previous stable version. and jshint . should execute jshint executable (which will give us an idea of what's wrong), please note that jshint executable and Atom package are two different things :)

jeffschwartz commented 8 years ago

Running jshint from the command line: ~/Development/preamble-ts-standalone$ jshint dist/core/htmlReporter/htmlReporter.js dist/core/htmlReporter/htmlReporter.js: line 15, col 9, Empty block. dist/core/htmlReporter/htmlReporter.js: line 17, col 51, 'data' is defined but never used.

2 errors

jeffschwartz commented 8 years ago

Sure, I can do that and report back.

jeffschwartz commented 8 years ago

Getting this error when running apm install linter-jshint@1:

 ~/Development/preamble-ts-standalone$ apm install linter-jshint@1
Installing linter-jshint@1 to /Users/Jeff/.atom/packages ✗
Package version: 1 not found
jeffschwartz commented 8 years ago

I ran apm install linter-jshint@1.3.1 which worked. Errors are being flagged in the gutter as expected but tooltips along with other atom linter configuration options are not working so perhaps the issue is atom linter (v1.11.3)?

jeffschwartz commented 8 years ago

I updated to v2.0.0 and errors are no longer being flagged using the same .js file as previously used with v1.3.1.

jeffschwartz commented 8 years ago

btw i'm running Atom on a Mac :)

jeffschwartz commented 8 years ago

One more piece of information: Atom Linter configuration options are all working with linter-tslint.

eruizdechavez commented 8 years ago

I am getting the same problem as @jeffschwartz, upgrading to v2 makes the issue reports to stop working completely. Downgrading to v1.3.1 restores functionality.

ghost commented 8 years ago

I also face the same situation as @eruizdechavez. I use win7.

Arcanemagus commented 8 years ago

Can anyone provide a file (+ configuration, if necessary) That this is reproducible on?

@jeffschwartz: Removed your last comment, looked like you pasted in a conversation from another forum?

eruizdechavez commented 8 years ago

I guess I can help trace this, just let me know what you need.

Arcanemagus commented 8 years ago

@eruizdechavez A file to reproduce this with, or more detailed steps to reproduce ~since it's working for me on the spec files :stuck_out_tongue:~

~Does apm test show any errors when run from the package folder?~

Edit, was confusing this with jscs, this doesn't have specs yet!

A file to reproduce this with will be helpful.

eruizdechavez commented 8 years ago

linter-jshint

eruizdechavez commented 8 years ago

@Arcanemagus apm test fails in for both 1.3.1 and 2; I guess I need some package for atom development.

EChavez-MBP:linter-jshint erick.ruizdechavez$ apm test
[15786:0216/140906:ERROR:renderer_main.cc(200)] Running without renderer sandbox
[15782:0216/140907:INFO:CONSOLE(85)] "Error: Cannot find module '/Users/erick.ruizdechavez/.atom/packages/linter-jshint/spec'
  at Module._resolveFilename (module.js:336:15)
  at Function.Module._resolveFilename (/Applications/Atom.app/Contents/Resources/app.asar/src/module-cache.js:383:52)
  at Function.Module._load (module.js:286:25)
  at Module.require (module.js:365:17)
  at require (/Applications/Atom.app/Contents/Resources/app.asar/src/native-compile-cache.js:50:27)
  at requireSpecs (/Applications/Atom.app/Contents/Resources/app.asar/spec/jasmine-test-runner.js:101:7)
  at module.exports (/Applications/Atom.app/Contents/Resources/app.asar/spec/jasmine-test-runner.js:49:7)
  at module.exports (/Applications/Atom.app/Contents/Resources/app.asar/src/initialize-test-window.js:71:17)
  at setupWindow (file:///Applications/Atom.app/Contents/Resources/app.asar/static/index.js:86:5)
  at window.onload (file:///Applications/Atom.app/Contents/Resources/app.asar/static/index.js:41:9)
", source: /Applications/Atom.app/Contents/Resources/app.asar/src/initialize-test-window.js (85)
Tests failed
Arcanemagus commented 8 years ago

apm test fails since there aren't any implemented, sorry about mentioning that, was confusing this with the other JS linter that I don't use :stuck_out_tongue:.

Testing this out myself right now...

Arcanemagus commented 8 years ago

@eruizdechavez What version of Atom are you using? linter-jshint@2.0.0 works perfectly fine for me on your test file :confused:

image

Arcanemagus commented 8 years ago

What is the output of atom.config.get('linter-jshint.executablePath') when you type that into Atom's Developer Tools?

eruizdechavez commented 8 years ago

Atom 1.5.3

EChavez-MBP:linter-jshint erick.ruizdechavez$ apm -v
apm  1.6.0
npm  2.13.3
node 0.10.40
python 2.7.10
git 2.5.4
eruizdechavez commented 8 years ago
EChavez-MBP:linter-jshint erick.ruizdechavez$ apm ls | grep linter
├── linter@1.11.3
├── linter-jshint@2.0.0
├── minimap-linter@1.1.1
eruizdechavez commented 8 years ago
EChavez-MBP:~ erick.ruizdechavez$ npm ls -g | grep jshint
├─┬ jshint@2.8.0
jeffschwartz commented 8 years ago

atom.config.get('linter-jshint.executablePath') "/Users/Jeff/.atom/packages/linter-jshint/node_modules/jshint/bin/jshint"

Arcanemagus commented 8 years ago

I can't reproduce on Atom 1.5.3 on Ubuntu either :confused: image

(First screenshot was from Windows 10)

@jeffschwartz That looks correct...

Arcanemagus commented 8 years ago

Can you guys place a breakpoint here to see if the lint() function is even being called.

Place one here, and see what result contains.

Place one here to see what parsed contains.

Arcanemagus commented 8 years ago

Ah ha! Do you guys have Lint Inline Java Script enabled? When that is enabled it shows no errors for that test file, there seems to be a pretty bad bug with that option from my investigation here.

eruizdechavez commented 8 years ago

Eureka!

Yup, after disabling it, all starts working again. (I probably enabled it when messing with my settings, I do not really need it).

Arcanemagus commented 8 years ago

As you can see, this is a bug in linter-jshint, but jshint itself: image

@jeffschwartz are you seeing the same thing?

Arcanemagus commented 8 years ago

@eruizdechavez If you are able to I would highly recommend switching to a modern JS linter like eslint or jscs.

eruizdechavez commented 8 years ago

@Arcanemagus I hear you!

eruizdechavez commented 8 years ago

@Arcanemagus and thanks for helping with this issue

Arcanemagus commented 8 years ago

I'm just glad it was traced down (probably, pending confirmation by @jeffschwartz)!

jeffschwartz commented 8 years ago

From the responses then it appears that a breaking change was introduced in v2.0.0. It cannot be jshint itself because it works with the previous version of the plugin.

Arcanemagus commented 8 years ago

@jeffschwartz Looking at the code there is a change related to this here, the textEditor.getGrammar().scopeName.indexOf('text.html') isnt -1 check wasn't migrated over to here, which is causing this jscs bug to become exposed on a wider scale.

Do you want to submit a PR rectifying that?

jeffschwartz commented 8 years ago

I don't see how --extract=always has anything to do with this issue as the option only impacts in-line javascript and does not impact javascript in .js files. I haven't tried it but my suspicion tells me that linter-jshint is also broken for in-line javascript as well.

jeffschwartz commented 8 years ago

@Arcanemagus if I had the time, prior experience with CoffeeScript and a previous exposure to Atom development I would love to but I have none of those :(.

Arcanemagus commented 8 years ago

@jeffschwartz Check the last screenshot posted: Enabling that option completely breaks jscs.

The change between v1.3.1 and v2.0.0 is that previously when that option was enabled it would only call that when the file had the text.html scope, now it always adds it.

I'll see if I have some time later to put up a PR adding that check back in to minimize the amount this bug is exposed.

jeffschwartz commented 8 years ago

@Arcanemagus I see that. Just curious but wouldn't a sanity/QA test have detected the issue and prevented the build from being released? Anyway, I really appreciate all the hard work you guys do in supporting and maintaining this plugin. It is immensely valuable in my development and I am sure I speak for many others who are using it as well :)

Arcanemagus commented 8 years ago

Of course it would, but this package has no active maintainer so it only has been getting brief bits of attention when @steelbrain or I have the time. A PR implementing specs would be wonderful :wink:

Arcanemagus commented 8 years ago

Can you check v2.0.1? It should be just as broken as v1.3.1 was :stuck_out_tongue:

The brokenness is within the --extract functionality though, so nothing we can really do about that here.

jeffschwartz commented 8 years ago

@Arcanemagus no tdd or bdd specs necessary for a sanity test/QA test. Just open a .js file in Atom with known errors and warnings. At that point, linting either works or it doesn't. Like I said previously, as I don't know CoffeeScript nor have I any Atom dev experience I wouldn't be the candidate to issue the pr for this since it should at least in my opinion be done ASAP because the production release of the plugin is broken. I really wish Atom had been developed using either pure JavaScript or TypeScript as that is where my expertise are. Again, my thanks go out to your and anyone else maintaining this project and I am sorry that I can't be of more help on the fix for this.

eruizdechavez commented 8 years ago

@Arcanemagus :+1:

Arcanemagus commented 8 years ago

@jeffschwartz There is no CoffeeScript in here as of v2.0.0, it's pure ES6 :wink:.

jpoo90 commented 8 years ago

I'm having the same problem as @jeffschwartz the linter stopped working after I updated to v 2.0. Today I updated to v 2.0.2 but the problem is still there.

I've tried restarting atom, enabling and disabling the linter without any luck. If I go back to v 1.3.1 it works again.

I'm using Atom 1.5.3 with ├── linter@1.11.3 ├── linter-jshint@2.0.2

Thanks in advance for any suggestions.

eruizdechavez commented 8 years ago

@jpoo90 have you tried to disable the option Lint Inline Java Script and see if it works?

jpoo90 commented 8 years ago

@eruizdechavez I did try that, but it doesn't work either :/

Thanks for the suggestion.

Arcanemagus commented 8 years ago

@jpoo90 can you share your .jshintrc file, an example file that you think should have errors but isn't showing anything, and the linter-jshint section of your configuration? (Or a screenshot of the Atom settings page for it works just as well for that)