mozilla / security-advisor-shield-study

Mozilla Public License 2.0
2 stars 7 forks source link

Add eslint-plugin-promise to check for promise ESLint issues? #47

Open pdehaan opened 7 years ago

pdehaan commented 7 years ago

Currently not super interesting...

Re: https://github.com/xjamundx/eslint-plugin-promise

And here's my diff (on top of my #46 PR):

diff --git a/.eslintrc.yaml b/.eslintrc.yaml
index 72c53b6..8e419f0 100644
--- a/.eslintrc.yaml
+++ b/.eslintrc.yaml
@@ -10,6 +10,7 @@ globals:

 plugins:
   - mozilla
+  - promise

 root: true

@@ -22,6 +23,11 @@ rules:
   mozilla/no-aArgs: warn
   mozilla/this-top-level-scope: warn

+  promise/always-return: error
+  promise/catch-or-return: error
+  promise/no-native": off
+  promise/param-names: error
+
   class-methods-use-this: off
   eqeqeq: error
   indent: [warn, 2, {SwitchCase: 1}]
diff --git a/package.json b/package.json
index 4c9e60b..adef8c7 100644
--- a/package.json
+++ b/package.json
@@ -13,6 +13,7 @@
     "eslint-plugin-import": "^2.0.0",
     "eslint-plugin-jsx-a11y": "^2.2.2",
     "eslint-plugin-mozilla": "^0.0.3",
+    "eslint-plugin-promise": "^2.0.1",
     "eslint-plugin-react": "^6.3.0",
     "jpm": "1.0.7",
     "node-sass": "3.8.0",

... and my ESLint output:

$ npm run lint

> security-advisor@0.0.1 lint /Users/pdehaan/dev/github/mozilla/security-advisor-shield-study
> eslint .

/Users/pdehaan/dev/github/mozilla/security-advisor-shield-study/index.js
  21:3  error  Expected catch() or return                  promise/catch-or-return
  21:3  error  Each then() should return a value or throw  promise/always-return

/Users/pdehaan/dev/github/mozilla/security-advisor-shield-study/lib/Advisor.js
  167:27  error  'self' is not defined  no-undef

/Users/pdehaan/dev/github/mozilla/security-advisor-shield-study/lib/variations.js
   7:20  warning  Missing function expression name  func-names
  11:22  warning  Missing function expression name  func-names

✖ 5 problems (3 errors, 2 warnings)

... Where index.js:21 looks like this:

20: function main(options, callback) { // eslint-disable-line no-unused-vars
21:   xutils.generateTelemetryIdIfNeeded().then(() => {
22:     xutils.handleStartup(options, thisStudy);
23:  });
24: }

So I guess if generateTelemetryIdIfNeeded() could potentially fail or throw an error, we don't catch it in any way. :shrug:

Osmose commented 7 years ago

generateTelemetryIdIfNeeded actually cannot reject the promise, it can only throw: https://github.com/mozilla/shield-studies-addon-utils/blob/master/lib/index.js#L39

As for linting for unhandled promises, I think it's an interesting addition. We stopped using https://github.com/mozilla/eslint-config-normandy/ for some reason, but maybe we should start using that again for these studies, and add the plugin there?

mythmon commented 7 years ago

We stopped using eslint-config-normandy when we switched to the airbnb config. It might make sense to roll the recent changes to linting into eslint-config-normandy, and use that everywhere to make linting more similar between projects. We've started to diverge again since we keep overriding airbnb.