chore: Switch from JSHint/JSCS to ESLint #581

pdehaan · 2016-09-20T00:34:08Z

Fixes #571

pdehaan · 2016-09-20T00:44:47Z

Other fun tools: depcheck, which reports potentially unused dependencies...

Currently reports the following:

$ depcheck

Unused dependencies
* commander
* deepcopy
* jpm-core
* jsonwebtoken
* lodash.merge
* node-watch
* object-assign
* open
* request

Unused devDependencies
* husky
* release-it

Missing dependencies
* sdk
* chrome

commander is clearly used in /bin/jpm:5 -- but that file has no extension, so probably why it wasn't found.
deepcopy seems unused, per GitHub search results
jpm-core seems used via require.resolve() in /lib/xpi.js:64.
jsonwebtoken removed in this PR.
lodash.merge removed in this PR -- but we already include all lodash anyways, so probably unnecessary anyways.
object-assign is clearly used in /bin/jpm:4 -- but that file has no extension...
open again in /bin/jpm:12.
request seems unused, per GitHub search results and git grep request.

husky and release-it are probably deploy tools (or at least husky is a precommit hook).

sdk and chrome are Firefox versions, I imagine, so can be ignored.

freaktechnik · 2016-09-20T10:29:29Z

According to http://eslint.org/docs/user-guide/configuring#configuration-file-formats using .eslintrc without file ending is deprecated, wouldn't it make sense to add the file ending to ensure the config is future-proof?

Is there really a need for two targets? I only ever see lint used, never eslint.

pdehaan · 2016-09-20T15:59:09Z

According to http://eslint.org/docs/user-guide/configuring#configuration-file-formats using .eslintrc without file ending is deprecated, wouldn't it make sense to add the file ending to ensure the config is future-proof?

Good catch. I did that for the top level, but totally choked on the override config in test/.

Is there really a need for two targets? I only ever see lint used, never eslint.

A need? No. But it can make things a lot easier if we add more linting tools in the future. For example, if we ever add CSS linting [or whatever], we could just tweak the package.json's lint task and not have to edit .travis.yml, prepush hook, pretest hook, and other places. We've used it in other projects with some success (since every project has a common "lint" task, you don't need to think about it very hard). But I'm happy to remove if it's excessive here.

pdehaan · 2016-09-20T16:00:08Z

package.json

@@ -61,9 +58,8 @@
    "open": "0.0.5",
    "promzard": "0.3.0",
    "read": "1.0.7",
-    "request": "2.67.0",


Removed unused deps, per #581 (comment)

This is great but I'd like to make sure it doesn't happen again. Can we use this rule maybe?

No. I added that plugin and configured it, and then added the request dependency back and ESLint still passed, so it doesn't seem to do the same thing as depcheck.

We can probably just use depcheck either on Travis-CI only, or add it to devDependencies.

Hmm, I swear I've done this before (cant' find it) but maybe it was for a project only with ES6 imports.

ok, if you can run depcheck as part of the Travis build, that would be great

pdehaan · 2016-09-20T16:05:59Z

package.json

@@ -76,11 +72,10 @@
    "conventional-changelog-cli": "1.2.0",
    "conventional-changelog-lint": "1.0.0",
    "dive": "0.4.0",
+    "eslint": "2.13.1",


Oh, and I forgot that I should throw a comment on here...

I'm not sure what version of ESLint to use. Per http://eslint.org/docs/user-guide/migrating-to-3.0.0

With ESLint v3.0.0, we are dropping support for Node.js versions prior to 4. Node.js 0.10 and 0.12 are in maintenance mode and Node.js 4 is the current LTS version. If you are using an older version of Node.js, we recommend upgrading to at least Node.js 4 as soon as possible. If you are unable to upgrade to Node.js 4 or higher, then we recommend continuing to use ESLint v2.x until you are ready to upgrade Node.js.

Important: We will not be updating the ESLint v2.x versions going forward. All bug fixes and enhancements will land in ESLint v3.x.

I wasn't 100% on our support for Node < 4 (the package.json mentions node>=0.10), so I left this as eslint@2. If we're only supporting Node >= 4, we should probably upgrade to eslint@3.

As travis only runs 4+, and this is a devDependency this shouldn't be a problem, there are already devDependencies that only support node 4 and up (get-firefox as an example).

Updated to eslint@3 and re-pushed.

kumar303

Thanks, this is really helpful! The original contributor who added jshint went with that since he didn't know eslint even though that's what I had wanted. I wasn't trying to be picky about a contribution though :)

I requested just a few changes but let me know if adding the dep-check rule is too much for this patch (we can do it later).

kumar303 · 2016-09-21T18:50:21Z

package.json

@@ -16,10 +16,10 @@
    "addon_runners": "node ./test/run.js addon_runners",
    "unit": "node ./test/run.js unit",
    "documentation": "mocha -t 5000 test/documentation",
-    "jshint": "jshint bin/jpm lib test --exclude test/addons,test/addon_runners,test/fixtures",
-    "jscs": "jscs bin/jpm lib",
+    "eslint": "eslint bin/jpm lib test",


I think the lint command is enough, right? I'd like to avoid aliases if we don't need them.

Removed eslint alias and just left lint (which still lets me be consistent with our other repos). 👍

kumar303 · 2016-09-21T18:53:45Z

package.json

@@ -61,9 +58,8 @@
    "open": "0.0.5",
    "promzard": "0.3.0",
    "read": "1.0.7",
-    "request": "2.67.0",


This is great but I'd like to make sure it doesn't happen again. Can we use this rule maybe?

kumar303 · 2016-09-21T18:59:15Z

hmm, I'm not sure what these test failures are about

pdehaan · 2016-09-21T20:06:25Z

hmm, I'm not sure what these test failures are about

I think this broke when I added the eqeqeq rule, but not sure what would have caused that. Unless it's the return (process.env.TRAVIS == "true");.

All the other changes in that commit were just comparing to strings or numbers. I'll see if switching back to == fixes the failing Travis.

Computers are hard.

pdehaan · 2016-09-21T20:57:18Z

Just talking to myself here while my Travis-CI is running in the background.

So, with depcheck, we can get it to ignore certain dependencies using the --ignores flag (per usage docs).

Not super pretty though:

$ depcheck --ignores=commander,jpm-core,node-watch,object-assign,open,eslint-plugin-*,husky,release-it

Missing dependencies
* sdk
* chrome

$ echo $? # 0

So it doesn't seem to know how to handle the quirky sdk and chrome faux dependencies, ~~and always seems to exit with an exit code of 0 (so it won't fail travis ever, seemingly).~~ Actually, it seems the error code was fixed in the newer [email protected], but still gives an exit code of 255 for the missing sdk and chrome dependencies, so we'd need to figure that config out, or file an upstream bug.

UPDATE: Filed upstream depcheck issue as depcheck/depcheck#167

Also, doesn't look like an easy/good way to pretty up that horrible --ignores= list on the CLI yet, but we may be able to use a wrapper like https://github.com/mcasimir/depcheck-ci which hides it all in a config file.

freaktechnik · 2016-09-21T21:05:18Z

I think the proper fix to ignore the sdkand chrome dependencies would be to disable the depcheck in data/, since that doesn't have any actual requiring in there.

kumar303 · 2016-09-21T21:13:54Z

The latest failure looks like a flaky timeout so I restarted it

pdehaan · 2016-09-21T21:17:00Z

I think the proper fix to ignore the sdkand chrome dependencies would be to disable the depcheck in data/, since that doesn't have any actual requiring in there.

That'd solve 2/3 of the problems. We'd still need a solution for the require('chrome') in test/fixtures/.../addon/runner.js:

Per our mozilla-jetpack/jpm repo, we have the following files (and special dependencies):

data/index.js:1 — var self = require("sdk/self");

data/test-index.js:19 — require("sdk/test").run(exports);

test/fixtures/mock-sdk/lib/sdk/addon/runner.js:5 — const { Cc, Ci } = require("chrome");

— via depcheck/depcheck#167

But yes, works great for the two require('sdk/self') issues, by adding --ignore-dirs=data:

$ ./node_modules/.bin/depcheck --ignores=commander,jpm-core,node-watch,object-assign,open,eslint-plugin-*,husky,release-it,depcheck --ignore-dirs=data

Missing dependencies
* chrome

I can't seem to find the right combination of getting this to work, although my inferior brain seems to think it should:

--ignore-dirs=data,test/fixtures/mock-sdk/lib/sdk/addon

Maybe it's choking on the / in the second path.

UPDATE: My assumption was correct, it fails on nested directories: depcheck/depcheck#138

pdehaan · 2016-09-21T21:32:51Z

In this new "GitHub review" world, is there a way that I can mark the requested changes as complete? Or can only repo owners/admins/members do that?

TL;DR: Merge this quick while it's green!

kumar303 · 2016-09-21T22:23:41Z

Thanks @pdehaan !

Can you file a new issue to fix the TODOs that you commented about in .eslintrc.yml? I think those are all valid, I'd especially like to turn on no-console and max-len.

pdehaan · 2016-09-22T15:32:48Z

Can you file a new issue to fix the TODOs that you commented about in .eslintrc.yml? I think those are all valid, I'd especially like to turn on no-console and max-len.

Filed #585 which is the overall TODO issue, and submitted #587 which is the PR to fix no-console and max-len. 👀

chore: Switch from JSHint/JSCS to ESLint

bc4b03f

chore: Remove reference to JSHint in Travis and tests

0efadb5

pdehaan added 2 commits September 20, 2016 08:50

chore: Fix eslint config file extension in test/ and add eqeqeq rule

d6c7f8f

chore: Remove unused dependencies from package.json

03d8f8c

pdehaan commented Sep 20, 2016

View reviewed changes

chore: Update ESLint to v3

0063985

kumar303 suggested changes Sep 21, 2016

View reviewed changes

chore: add eslint-plugin-dependencies plugin and fix test error

6b6cf86

kumar303 approved these changes Sep 21, 2016

View reviewed changes

kumar303 merged commit 5f7a5ac into mozilla-jetpack:master Sep 21, 2016

pdehaan deleted the yay-eslint branch September 21, 2016 22:38

pdehaan restored the yay-eslint branch September 22, 2016 15:32

pdehaan deleted the yay-eslint branch September 22, 2016 15:32

rpl mentioned this pull request Sep 22, 2016

Feature: add support for hybrid SDK addons #578

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Switch from JSHint/JSCS to ESLint #581

chore: Switch from JSHint/JSCS to ESLint #581

pdehaan commented Sep 20, 2016 •

edited

Loading

pdehaan commented Sep 20, 2016

freaktechnik commented Sep 20, 2016

pdehaan commented Sep 20, 2016

pdehaan Sep 20, 2016

kumar303 Sep 21, 2016

pdehaan Sep 21, 2016

kumar303 Sep 21, 2016

kumar303 Sep 21, 2016

pdehaan Sep 20, 2016

freaktechnik Sep 20, 2016

pdehaan Sep 20, 2016

kumar303 left a comment

kumar303 Sep 21, 2016

pdehaan Sep 21, 2016

kumar303 Sep 21, 2016

kumar303 commented Sep 21, 2016

pdehaan commented Sep 21, 2016

pdehaan commented Sep 21, 2016 •

edited

Loading

freaktechnik commented Sep 21, 2016

kumar303 commented Sep 21, 2016

pdehaan commented Sep 21, 2016 •

edited

Loading

pdehaan commented Sep 21, 2016

kumar303 commented Sep 21, 2016

pdehaan commented Sep 22, 2016

chore: Switch from JSHint/JSCS to ESLint #581

chore: Switch from JSHint/JSCS to ESLint #581

Conversation

pdehaan commented Sep 20, 2016 • edited Loading

pdehaan commented Sep 20, 2016

freaktechnik commented Sep 20, 2016

pdehaan commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 commented Sep 21, 2016

pdehaan commented Sep 21, 2016

pdehaan commented Sep 21, 2016 • edited Loading

freaktechnik commented Sep 21, 2016

kumar303 commented Sep 21, 2016

pdehaan commented Sep 21, 2016 • edited Loading

pdehaan commented Sep 21, 2016

kumar303 commented Sep 21, 2016

pdehaan commented Sep 22, 2016

pdehaan commented Sep 20, 2016 •

edited

Loading

pdehaan commented Sep 21, 2016 •

edited

Loading

pdehaan commented Sep 21, 2016 •

edited

Loading