Complete ESLint conversion and cleanup #490

MitMaro · 2018-06-14T18:41:23Z

@JacoKoster started using ESLint in Pull Request #480, this change completes the work stared in that PR.

There are three commits to this PR that are related but I will gladly separate into separate PRs if desired.

ESLint config rename

The first change is to rename the ESLint configuration file to use a file extension as the config file without an extension was deprecated several years ago. source

Running ESLint

The second change adds ESLint as a development dependency for the project and adds a lint script to run ESLint against the project. This should also make it so that the TravisCI will also run the linting script.

JSHint to ESLint conversion

This is the largest change in the PR. This removes the JSHint configuration file and adds ESLint rules that are equivalent. In the process there were a few files in test/ that were failing the linting process that had to be fixed.

Changes

`"evil": true`

In JSHint this made sure eval was not used. It is replaced with the equivalent "no-eval": "error" and "no-implied-eval": "error". ESLint detects a bit more than what JSHint did in this regard.

`"regexdash": true`

This was removed some time ago from JSHint, but it caught "an unescaped last dash (-) inside brackets". It is replaced with "no-div-regex": "error", "no-invalid-regexp": "error", and "no-control-regex": "error". The ESLint rules go further than the JSHint rule in validating regular expressions.

`"browser": false`

With JSHint this added some globals around the use in the browser. This was set to false in the configuration, so was not enabled. By default ESLint does not add these globals, and instead is added using the browser environment configuration.

`"wsh": true`

This adds globals for Windows Script Host, and does not make sense in this project, and I would assume was not meant to be added.

`"trailing": true`

This ensures no trailing whitespave on files, and was removed from JSHint. JSHint recommended using JSCS for this rule, which was eventually merged into ESLint. The "no-trailing-spaces": "error" ESLint rule is a direct replacement for this rule.

`"sub": true`

Like trailing this rule has been moved to JSCS, and is a direct replacement with "dot-notation": "error" in ESLint.

`"unused": true`

Warns on unused variables, This is directly replaced with "no-unused-vars": "error" in ESLint. The ESLint rule also catches some cases the JSHint does not, as seen in the updates required to the tests.

`"undef": true`

Warns on using variables before they are defined, replaced with "no-undef": "error" in ESLint.

`"laxcomma": true`

Another rule that was moved to JSCS, and can be directly replaced with "comma-style": "error" from ESLint.

`"node": true`

Adds globals that are only available in Node projects, directly replaced with "node": true in env in ESLint

`"esnext": true`

Adds support to JSHint for ECMAScript 6, directly replaced with "es6": true in env and "ecmaVersion": 6 in parserOptions.

Globals `"describe": true`, `"it": true`, `"before": true`

Replaced with "mocha" true in env which adds the same globals

Global `"require": true`

Added with "node": true in env in ESLint

Globals `"atob": false` and `"escape": true`

These are considered built-ins and are allowed by default in ESLint.

JacoKoster · 2018-06-14T19:06:54Z

Excellent work, with the move to ES6, i would also really-really like to deprecate the older node versions.

This fixes #486, btw.

MitMaro · 2018-06-14T19:16:56Z

I read that issue when you posted it but forgot all about it. I hope I didn't step on any work you may have been doing.

JacoKoster · 2018-06-14T19:23:28Z

No worries, i was busy figuring out how to use ecdsa-certificates with JWKS and this library. If i get that working i was planning on doing the OpenID improvements

JacoKoster · 2018-06-19T13:59:27Z

In another project the linter gave an error about shadowed variables. I have seen that also as comments on PR's in this project, so i recommend adding no-shadow as well ;-)

MitMaro · 2018-06-19T14:08:14Z

I limited this PR to only contain the conversions from JSHint to ESLint. I do agree that there are a lot of other rules that could be added, including no-shadow, and I wouldn't mind putting up a PR with some of those rules later. Assuming that this PR can be merged. :)

JacoKoster · 2018-06-19T14:29:52Z

@ziluvatar Care to join us in the discussion?

ziluvatar · 2018-06-20T14:07:32Z

I'd like to review both JSHint and ESLint rules (I don't have too much knowledge about them) and the changes in this PR before merging. The move JSHint to ESLint is fine for me, just give me some time to investigate :)

MitMaro · 2018-06-20T14:15:15Z

@ziluvatar I will try to add some links to the PR description that will help in your investigations this evening. :)

JacoKoster · 2018-06-20T18:59:29Z

package.json

@@ -4,7 +4,8 @@
  "description": "JSON Web Token implementation (symmetric and asymmetric)",
  "main": "index.js",
  "scripts": {
-    "test": "mocha --require test/util/fakeDate && nsp check && cost-of-modules"
+    "lint": "eslint .",
+    "test": "mocha --require test/util/fakeDate && npm run lint && nsp check && cost-of-modules"


In #486 It was mentioned to break the build early when linting fails and it was therefor proposed to move the lint in front of the test.

I've moved linting first.

The .eslintrc file without an extension was deprecated a few years ago, so this change renames the file to add the required extension. See: eslint/eslint@c9a8883

This change adds ESLint as a dev-dependency and adds a lint script that will run ESLint.

Convert all the JSHint rules to the ESLint equivalents where possible. The no-undef rule in ESLint caught a few cases of undefined usages in the tests, so they were also fixed.

MitMaro · 2018-06-20T19:27:57Z

I've rebased the branch to fix the conflict.

MitMaro · 2018-06-20T20:00:45Z

I've updated the PR description with some more information on each conversion. I will gladly go into more detail on anything if needed.

JacoKoster · 2018-06-20T20:29:26Z

.eslintignore

@@ -0,0 +1,3 @@
+.nyc_output/
+coverage/
+node_modules/


In addition to any patterns in a .eslintignore file, ESLint always ignores files in /node_modules/* and /bower_components/*.

https://eslint.org/docs/user-guide/configuring#ignoring-files-and-directories

Thanks! Removed.

The HTML coverage report is currently being linted, which causes a lot if invalid linting errors. This change adds a ignore file to ensure these files are properly skipped during linting.

ziluvatar

Awesome work @MitMaro when I came back and saw how you modified the description ❤️

ziluvatar · 2018-06-25T17:50:57Z

.eslintrc.json

+    "no-invalid-regexp": "error",
+    "no-trailing-spaces": "error",
+    "no-undef": "error",
+    "no-unused-vars": "error"


Maybe this could be ignored in tests, where you usually don't use all the variables from a function. For now it is fine since you fixed them. Let's keep that in mind now that you will play a lot with tests.

MitMaro changed the title ~~Complete Eslint conversion and cleanup~~ Complete ESLint conversion and cleanup Jun 14, 2018

MitMaro force-pushed the eslint-cleanup branch from 116ab32 to ae9f38a Compare June 14, 2018 18:43

JacoKoster mentioned this pull request Jun 19, 2018

Build pipeline #486

Closed

JacoKoster reviewed Jun 20, 2018

View reviewed changes

MitMaro added 3 commits June 20, 2018 16:49

Add extension to ESLint config file

94ad6d4

The .eslintrc file without an extension was deprecated a few years ago, so this change renames the file to add the required extension. See: eslint/eslint@c9a8883

Add ESLint to package.json

1e186e4

This change adds ESLint as a dev-dependency and adds a lint script that will run ESLint.

Complete switch from JSHint to ESLint

130eca3

Convert all the JSHint rules to the ESLint equivalents where possible. The no-undef rule in ESLint caught a few cases of undefined usages in the tests, so they were also fixed.

MitMaro force-pushed the eslint-cleanup branch from ae9f38a to c12dfb5 Compare June 20, 2018 19:27

MitMaro force-pushed the eslint-cleanup branch from c12dfb5 to 6e78e3e Compare June 20, 2018 20:03

JacoKoster reviewed Jun 20, 2018

View reviewed changes

Add a .eslintignore file

13cb09e

The HTML coverage report is currently being linted, which causes a lot if invalid linting errors. This change adds a ignore file to ensure these files are properly skipped during linting.

MitMaro force-pushed the eslint-cleanup branch from 6e78e3e to 13cb09e Compare June 21, 2018 10:40

ziluvatar approved these changes Jun 25, 2018

View reviewed changes

ziluvatar merged commit cb1d2e1 into auth0:master Jun 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete ESLint conversion and cleanup #490

Complete ESLint conversion and cleanup #490

MitMaro commented Jun 14, 2018 •

edited

Loading

JacoKoster commented Jun 14, 2018 •

edited

Loading

MitMaro commented Jun 14, 2018

JacoKoster commented Jun 14, 2018 •

edited

Loading

JacoKoster commented Jun 19, 2018

MitMaro commented Jun 19, 2018

JacoKoster commented Jun 19, 2018

ziluvatar commented Jun 20, 2018

MitMaro commented Jun 20, 2018

JacoKoster Jun 20, 2018

MitMaro Jun 20, 2018

MitMaro commented Jun 20, 2018

MitMaro commented Jun 20, 2018

JacoKoster Jun 20, 2018 •

edited

Loading

MitMaro Jun 21, 2018

ziluvatar left a comment

ziluvatar Jun 25, 2018

Complete ESLint conversion and cleanup #490

Complete ESLint conversion and cleanup #490

Conversation

MitMaro commented Jun 14, 2018 • edited Loading

ESLint config rename

Running ESLint

JSHint to ESLint conversion

Changes

"regexdash": true

"trailing": true

Globals "describe": true, "it": true, "before": true

Global "require": true

Globals "atob": false and "escape": true

JacoKoster commented Jun 14, 2018 • edited Loading

MitMaro commented Jun 14, 2018

JacoKoster commented Jun 14, 2018 • edited Loading

JacoKoster commented Jun 19, 2018

MitMaro commented Jun 19, 2018

JacoKoster commented Jun 19, 2018

ziluvatar commented Jun 20, 2018

MitMaro commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitMaro commented Jun 20, 2018

MitMaro commented Jun 20, 2018

JacoKoster Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziluvatar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitMaro commented Jun 14, 2018 •

edited

Loading

`"regexdash": true`

`"trailing": true`

Globals `"describe": true`, `"it": true`, `"before": true`

Global `"require": true`

Globals `"atob": false` and `"escape": true`

JacoKoster commented Jun 14, 2018 •

edited

Loading

JacoKoster commented Jun 14, 2018 •

edited

Loading

JacoKoster Jun 20, 2018 •

edited

Loading