Investigate ESLint #235
Comments
|
@ariel-phet suggested to defer to an upcoming meeting for scheduling work on this. |
|
ESLint will probably take longer to process than JSHint since it produces an AST for each file. |
|
I'm interested to investigate this, but don't think I'll have time until after Bending Light is published. @pixelzoom said this may not be worth the effort at the moment, may be more important if/when we re-engage with 3rd parties. @samreid suggested investigating this when someone is reviewing a simulation and wishing for better automated support. |
|
ESLint 1.4.0 was released recently, with support for cached results which will improve speed significantly (linting only occurs on files that have changed since last time of lint). Come to think of it, this may even be a speed improvement over our jshint process. So my concerns regarding performance are significantly allayed. |
|
I've been enjoying the IDEA jshint plugin and noticed it has a similar ESLint plugin. It would be very nice to catch ESLint issues in the IDE while coding. Thinking about how to approach ESLint--we wouldn't have to immediately bail on jshint and jump ship to ESLint, we could add an eslint phase to our existing process that only checks for specific things (that we miss with jshint). I just saw another file where the constructor name didn't match the filename and it confused me for a bit. This seems like a natural thing to look for in ESLint--if there are any constructor functions, make sure at least one matches the filename. And I confirmed that ESLint context provides filename. |
|
@jbphet @jonathanolson say ok to investigate ESLint as an add-on step as long as it doesn't slow down the build. |
|
Assigned to myself (low priority) to take a look and see how this would impact our toolchain. |
|
Testing for ESLint performance, using 3 eslint tasks in bending-light I see: time grunt lint-all // 14.358s After introducing an error into one file (3 missing ;): So a cached eslint with a small number of modified files takes about 14% of the time of running jshint. Without caching (or if all files changed) the times seem comparable. A huge difference in the formatting: vs
For me the eslint output is far easier to grok. |
|
I'm elated at the idea of a linting step that takes 2 seconds instead of 14 seconds. Elated enough to abandon jshint completely and replace it with eslint? Perhaps. |
|
Guess who wouldn't have to deal with |
|
Checking for the cost (in time) of adding a cached eslint step to our existing process (default grunt, which includes jshint+requirejs+etc) with eslint: 51.014s So it looks like the time added by a cached eslint is in the noise. |
|
For reference, here are the default eslinting rules our code doesn't follow: |
|
I don't want to spend much time on this without getting a +1 from other devs, but I do want to commit my changes so that we can easily continue if we decide to. |
|
I posted this in the google group: https://groups.google.com/forum/#!forum/developing-interactive-simulations-in-html5 I'm investigating ESLint support in chipper as part of #235 . The package.json got 2 new dependencies, so developers and build processes will likely have to run Please report any issues here, and have a nice weekend! |
|
To get my eslint rule sealegs, I'm writing a rule that checks that each file has a copyright statement. Here are some typos/errors I'm seeing (and fixing): // Copyright 2002-2014, University of Colorado Boulder Boulder
// Copyright 2002-2014, University of Colorado
/*
* Copyright 2002-2014, University of Colorado Boulder
*/etc. |
|
Writing my own rule and applying it was a very positive experience. However, I'm unable to get IDEA to use my dynamic rules in its checking. It shows this error: |
|
Moving from "additional rules directory" to the options seems to have fixed this: |
|
Looks like some common-code repos do have |
|
After the above commits |
|
|
I had apparently picked up a transient version of chipper that had "auto-fix changes" turned on for ESLint, and it had changed a bunch of files, so my |
|
I changed grunt-all to use active-repos instead of active-runnables and went through a build to make sure all appropriate repos have eslint, and are linting properly. Optics lab is still an exception (as it was for jshint), but unless I have made a manual error, everything else is good to go. Next steps:
This "Investigate ESList" thread is getting a bit long and indeed our "investigation" is over, so I'll move the remaining work to a new issue and close this one. |
|
New issue created above, I'll close this one. |
|
One more answer to this question:
It appears if you change the .js code of a rule definition, it does *not invalidate caches. |
|
We could add support for deleting caches to the build process. But you can delete all caches like this: |
|
I also added a grunt command line argument --disable-eslint-cache which you can be used for iterating on rules. |
|
Does --disable-eslint-cache delete caches? Or just turn off caching? If the latter, that doesn't solve the problem. If a rule's code is changed, the caches need to be deleted. |
It just disables it for the current run, the cache is not deleted. |
|
@samreid please let me know what is the resolution on this? I am using windows machine. |
|
@AshrafSharf we have removed the line ending rule from the linting rules and will be discussing the issue shortly in #380 |
In #195 we determined that JSHint does not provide support new linting rules or tests. However, it seems like https://github.com/eslint/eslint is becoming very popular and it looks like it has backward support for JSHint rules and the ability to add your own rules.
If we are going to be adding more of our lint style rules like in #195 then we may want to invest in ESLint. No plans to look into it now but wanted to bring it up for future investigation.
The text was updated successfully, but these errors were encountered: