Switch to eslint for style & code checking allowing ES7 syntax #4040
Conversation
|
I'm not apposed to this, but would like a comparison chart of what we lose. If thats nothing then great, just want to be aware of the negative trade-offs. We also need an upgrade path for the existing jshint files etc. |
|
+1 I've already switched to ember-cli-eslint and most of my files start with I didn't miss anything from jshint yet. |
|
That is the table I came up with after checking https://github.com/jrajav/jshint2eslint, https://github.com/valorkin/eslint-rules-mapper the jshint and eslint docs:
For another big project switching you can check: https://www.drupal.org/node/2264205 Also some of the added rules make the style a bit stricter now such as |
|
As a note, it may be best to wait until after breaking changes in ESLint v1.0.0, which is likely not far off. See the milestone list for what is remaining. One large change is that rules will not be enabled by default, so it will be much easier to configure exactly the rules wanted. |
|
Another interesting new feature might be http://eslint.org/docs/developer-guide/shareable-configs |
|
Might want to look at making an ember-watson task to do an AST transform to convert the jshint syntax to eshint syntax. |
|
I have just seen this thread as a link from Slack; as ember-cli-eslint has been mentioned and shareable configs I thought I would chime in. I changed ember-cli-eslint to use eslint-config-ember shared config. My plan for this is to have a config which closely matches the communities defaults, it isn't perfect yet as it has had limited testing however I am hoping that the config can include any plugins to match the JSCS custom tests the community uses (Which seem to be covered by ESLint default rules - eslint/eslint#2206 << I think that was the last one missing which I can enable if people are interested) Along with my proposal to simplify testGeneration: ember-cli/rfcs#15 My personal motivation is getting people to having the simplest process for this: With an I would be very happy to open up the conversations about using the above setup to match what you guys would like to see as Embers defaults etc in the corresponding GitHub repos. |
|
@jonathanKingston nice, i prefer the add-on route. I think of ember-cli will support eslint, it will be via an add-on. Not directly like this. I will close this PR, I would accept a PR that extracted jshint similar to how @jonathanKingston has done with eslint. That way users can swap in-and out. |
|
I might be misreading this (it's late here) however I actually think the PR makes sense as well as the separate work to extract away JSHint as a plugin in projects. Ember(-cli) core could benefit from having linting that everyone is happy with (which the core could choose to have it's own linting custom setup for example) if that is the shared config as the core then even better. My comment was mostly in the direction of: whatever the outcome of this PR is I would like to unify all Ember apps to have a similar default. |
|
@stefanpenner Do you mean creating a jshint plugin for generated ember-cli apps? This PR was for ember-cli core code to switch to eslint. |
Yes, but that is a non-trivial upgrade for our users. Since all there custom + inherited rules would need to be updated. This seems like a pretty high short-term cost. What this PR should rather do. Extract jslint & eslint parts into addons, so they can easily be swapped. initially ship with jslint, and if eslint proves to be the more common choice we can swap over the default. But only once we have a streamlined and documented upgrade process. |
|
I would have expected that the ember-cli core team would want to specify the linting which is done on the core ember-cli code base (i.e. this repo), rather than allow it to be swapped out. There shouldn't be any work for users from this change, as jshint would still be included with new ember-cli projects just like before. I do really like the idea of being able to swap out linters on my ember-cli apps, but that seems like a separate effort. |
|
Or are you saying to give everyone a chance to get used to ESLint on their own apps via a plugin, and if most ember-cli contributors like it, then change over the ember-cli core? That would also make sense. |
Over at sane and a lot of other projects we switched to eslint.
It allows for style checking as well as static code analysis, but in a much more powerful way than jshint. Every rule is a plugin, so it is easy to add new rules to be checked and things such as complexity or code-smells can be checked in addition to a lot of the basic checks.
This makes jshint obsolete, since eslint is more or less capable of anything that jshint is (though it does not seem to be too easy to get a 1:1 rule conversion).
Furthermore eslint supports all the syntax that babel supports via babel-eslint fixing part of #3529.
Further to removing jshint I separated the linting out and added it as a
pretestmaking it possible tonpm run lintby itself and if linting fails it will not run any tests, making that faster.I took the rules we have at SANE as well as in parts at how-to-sane and slightly modified it to fit the existing ember-cli code as much as was easily possible.
To finish up this PR the rules should be confirmed so all the necessary errors can be confirmed. All rules can be found here: http://eslint.org/docs/rules/ and in they support
0for off,1for warning and2for error. Lastly the.eslintrcfiles in subfolders inherit all the rules from their parent folders.Let me know what you think and what I should do to finish up this PR.