feat(config): Deprecate JSHint in favor of ESLint

[feimosi]

This is the basic ESLint setup extending well-known Airbnb code style and dedicated Angular rules.
What also has changed:

• removed eslint task from client.views path in gulp watch, possibly we can think of using some html linter
• changed most of the globals by environment property, still I'd recommend not using / limit the use of global variables by dependency injection
• the previous contributor's rules are left in place, but we need to review them to make the migration seamless. For now it's not consistent with JSHint, so someone with longer experiences in the project could help convert all the rules - docs are quite well-written
• I haven't used Grunt much, so even though it was a basic change, someone could take a closer look :)

References #1072

[mleanos]

One thing to keep in mind is that we've decided on using John Papa's Angular Style-guide. I haven't taken a close look at this PR, but does the ESLint configuration take this into consideration? Or are there conflicting styles?

#943

[feimosi]

ESLint itself has only general purpose JS linting, but the plugin I've added has the ability to lint Angular apps. As they write in the summary: "some rules defined in John Papa's Guideline have been implemented". I just don't know how the defaults follow your conventions.

[codydaig]

@feimosi Thanks for the PR!

I am in favor of ESLINT, however, it doesn't match our style guide currently. There are a lot of linting errors, and the handful that I picked out and looked at I do not agree with.

Ex.)
In creating an object, it should not have a trailing comma
Incorrect: { foo: 'bar', }
Correct: { foo: 'bar',}

Ex.)
Strict mode is not permitted.
We want to have 'use strict' enabled.

Can you go through the errors and see if we can adjust the linter accordingly. If there are actual issues somewhere, fix those in a separate PR to make this PR pass the tests.

Thanks!

[feimosi]

@codydaig Ok, sure, I'll go through them when I find some time.

[rhutchison]

@codydaig, I am confused by this - typo?

Ex.)
In creating an object, it should not have a trailing comma
Incorrect: { foo: 'bar', }
Correct: { foo: 'bar',}

[rhutchison]

@feimosi I am in favor of this PR, but have questions about maintaining a single eslintrc file for this project.

I noticed that you have added the reference to angular default config, but I am curious what you have noticed when running these rules against the server-side files. Also, I have not seen much progress with adopting the style guide - are these rules failing?

Should nodejs lint rules be separated from angularjs rules? Can they be the same and still be restrictive enough?

[feimosi]

@rhutchison sorry, I wasn't available lately to take care of this PR.
Regarding the Angular config, I haven't noticed any conflicts so far. It looks like those rules are only fired for Angular-specific code. ESLint allows you to specify separate configs for each folder, but unfortunately not for filename patterns, nonetheless it's not a problem for now.
So I think we can keep both rule sets together.

[ilanbiala]

@feimosi still have lots of errors in the build. Can you take a look at them?

[ilanbiala]

These should be ~

[ilanbiala]

@feimosi can you rebase and fix up some of the changes requested above so we can merge?

[feimosi]

@ilanbiala I need some more time for this PR. Too busy these days.
Regarding angular-plugin, eventually I decided to remove it as I've noticed, in same cases it indeed interferes with node's code.

[trendzetter]

I had too disable eslint and rely only on jshint because it gave me too many warnings on code I migrated. The --fix option in the command line eslint is also broken so I couldn't fix the thousands of indentation errors which were cluttering my screen.
How do you guys correct styling errors automatically?

[ilanbiala]

@trendzetter I don't think anyone has corrected errors automatically.

[feimosi]

@trendzetter Last time I tried autocorrect it was working pretty badly, so I wouldn't rely on it.
@ilanbiala The config is ready. Now someone needs to review it. There are still ~150 errors, but I think they should be fixed and not muted by the rules. Also I had to turn off several rules following JSHint config, but I really think you should prepare a more strict style guide.

[ilanbiala]

@feimosi there shouldn't be any errors. JSHint never had any, so we should be able to switch to ESLint with whatever properties there and still not have any errors.

[feimosi]

@ilanbiala The linting rules are already very loose and there are many inconsistencies throughout the codebase. If I mute the given errors, the sense of using ESLint slowly fades out.

[trendzetter]

I could reduce the indentation and other styling errors for code I imported from close to 2000 to 10 using js-beautify
Then I fixed the remaining remarks manually. I am now running eslint with gulp again.

[lirantal]

It's quite obvious that this PR should bring with it actual fixes to all the errors and warnings so that eslint can actually go in.
@feimosi are you going to take care of it?

[lirantal]

@ilanbiala we can figure out which rules trigger errors and discuss on whether we should 'drop the rule' to make it compatible with what we have today or adopt that rule and make the necessary changes.
What do you think of this plan?

[ilanbiala]

Yeah, that sounds fine. First we need to rebase and go over each type of error we currently get. Though I'd rather rebase because some code will change and the errors may disappear/more may appear.

[feimosi]

That was exactly my point. ESLint introduces much more new rules, so they should be discussed by the core team and decided whether to keep them or not.

[ilanbiala]

@feimosi can you point us to the new ones/ones we don't use specifically? Also, can you rebase before we go any further?

[feimosi]

@ilanbiala These rules are failing:

comma-dangle
comma-spacing
consistent-return
default-case
guard-for-in
key-spacing
keyword-spacing
new-cap
no-alert
no-spaced-func
no-throw-literal
no-trailing-spaces
no-undef
no-unneeded-ternary
no-unreachable
one-var
one-var-declaration-per-line
quote-props
spaced-comment
warnings)
wrap-iife

You can always check it with this command:
eslint modules/**/*.js | awk 'NF>1{print $NF}' | sort | uniq

[ilanbiala]

@lirantal @mleanos final check before I merge?

[lirantal]

I'm all up for it.

[lirantal]

Just to add though that there are warnings we still have to deal with, such as:

[ilanbiala]

@lirantal I think that's fine that we have console statements and putting the comment above seems like clutter, no?

[ilanbiala]

@feimosi can you rebase and squash?

[lirantal]

@ilanbiala yeah not the end of the world.

[feimosi]

Done, you'd better hurry with that merge. It starts to get boring fixing the merge conflicts all the time.

[ilanbiala]

@feimosi there are some build errors, can you take a look?

[ilanbiala]

Please delete JSHint dependencies too and remove .jshintrc.

[feimosi]

@ilanbiala everything's done

[mleanos]

I just restarted the build. Looks like a connectivity/time-out error.

[ilanbiala]

LGTM, merging. @feimosi thanks for the hard work and following up on this every time!

[mleanos]

@feimosi Yes, thank you for all the hard work! No more complex/unnecessary ternary's, among other styling issues you fixed here ;)

[feimosi]

Thanks too, I'm glad I could help! :)

[codydaig]

WooHoo!!

[trendzetter]

I am not happy, has any of you written any amount of code with this enabled? It's really unbearable. It should be possible to turn this strict cosmetic mode off when you are not writing code for the meanjs framework itself. If you are writing application logic this is just too distracting, you just want to get something moving first and then brush it up.
How do I turn off specific eslint rules easily?
Why are rules like http://eslint.org/docs/rules/no-console enabled? The node app is intentionally writing to the console so this rule will be triggered all the time.
Plus it contains at least 2 bugs (#1319) probably not tested well.

[simison]

That extra space breaks indentation here, now space to the next level is 1 space. Was it required by eslint? Should the whole block be moved by one space instead?

[trendzetter]

These lint errors are making me angry, sorry for the ranting. I found I can skip rules by setting them them 0 in .eslintrc rules. I am giving it another shot at cleaning things up with the best beautification tricks found so far.

[trendzetter]

Just tried the eslint cli again and it has improved in recent times. It now is able to fix some issues automatically using the --fix flag. You would expect it to fix more issues automatically but at least its a beginning.