Upgrade eslint to 4.7.0 & update rules #41

ismail-syed · 2017-09-18T17:45:19Z

Updated eslint to 4.7.0
Added missing rules from previous eslint releases
Updated out of date plugins as per david-dm listing

Rules descriptions tagged with <FEEDBACK> could use some discussion/feedback.

:

ismail-syed · 2017-09-18T17:53:12Z

lib/config/rules/import.js

@@ -75,4 +75,6 @@ module.exports = {
  'import/max-dependencies': 'off',
  // Forbid unassigned imports
  'import/no-unassigned-import': 'off',
+  // Forbid anonymous values as default exports <FEEDBACK>
+  'import/no-anonymous-default-export': 'off',


Rule docs

Thoughts on this, IIRC I've seen our code export defaults before. Are there any specific types we should explicitly error?

I think we can turn this on, we prefer named functions anyways, and exporting literals is a pretty uncommon case.

ismail-syed · 2017-09-18T17:54:11Z

lib/config/rules/node.js

@@ -7,6 +7,8 @@ module.exports = {
  'global-require': 'off',
  // Enforces error handling in callbacks
  'handle-callback-err': ['warn', '^.*(e|E)rr(or)?$'],
+  // disallow use of the Buffer() constructor
+  'no-buffer-constructor': 'error',


security vulnerability, see https://eslint.org/docs/rules/no-buffer-constructor

ismail-syed · 2017-09-18T17:58:41Z

lib/config/rules/react.js

@@ -1,8 +1,12 @@
 module.exports = {
  // General

+  // Enforces consistent naming for boolean props <FEEDBACK>
+  'react/boolean-prop-naming': 'off',


This seems useful but could cause some frustrations adopting in a large code base. Doesn't seem to have a fixer either.

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/boolean-prop-naming.md

It would be cool if we could do the opposite of the default (never start with is/ has), as this is our standard.

What the reasoning behind that being our standard?

🤷‍♂️ it's just kind of what we landed on, it's consistent with our Ruby components and it avoids some kind of unnecessary duplication from my perspective

ismail-syed · 2017-09-18T18:03:41Z

lib/config/rules/stylistic-issues.js

@@ -129,6 +135,8 @@ module.exports = {
  'operator-linebreak': ['warn', 'after', {overrides: {'?': 'before', ':': 'before'}}],
  // Enforce padding within blocks
  'padded-blocks': 'off',
+  // require or disallow padding lines between statements <FEEDBACK>
+  'padding-line-between-statements': 'off',


This rule has lots of config options: https://eslint.org/docs/rules/padding-line-between-statements
I'd say just keep it off.

lemonmade

Looks great, left some comments but Im in agreement on almost everything

lemonmade · 2017-09-18T18:09:34Z

lib/config/rules/import.js

@@ -75,4 +75,6 @@ module.exports = {
  'import/max-dependencies': 'off',
  // Forbid unassigned imports
  'import/no-unassigned-import': 'off',
+  // Forbid anonymous values as default exports <FEEDBACK>
+  'import/no-anonymous-default-export': 'off',


I think we can turn this on, we prefer named functions anyways, and exporting literals is a pretty uncommon case.

lemonmade · 2017-09-18T18:10:31Z

lib/config/rules/possible-errors.js

+  // enforce “for” loop update clause moving the counter in the right direction.
+  'for-direction': 'error',
+  // enforce return statements in getters <FEEDBACK>
+  'getter-return': ['error', {'allowImplicit': true}],


No need for the quotes on allowImplicit, and 👍

lemonmade · 2017-09-18T18:12:35Z

lib/config/rules/react.js

@@ -1,8 +1,12 @@
 module.exports = {
  // General

+  // Enforces consistent naming for boolean props <FEEDBACK>
+  'react/boolean-prop-naming': 'off',


It would be cool if we could do the opposite of the default (never start with is/ has), as this is our standard.

lemonmade · 2017-09-18T18:18:05Z

lib/config/rules/react.js

@@ -78,6 +88,8 @@ module.exports = {
  'react/jsx-boolean-value': 'warn',
  // Validate closing bracket location in JSX
  'react/jsx-closing-bracket-location': ['warn', {location: 'tag-aligned'}],
+  // Validate closing tag location in JSX
+  'react/jsx-closing-tag-location': 'error',


I think we want line-aligned for the option here

lemonmade · 2017-09-18T18:18:35Z

lib/config/rules/stylistic-issues.js

@@ -1,8 +1,12 @@
 // see http://eslint.org/docs/rules/#stylistic-issues

 module.exports = {
+  // enforce linebreaks after opening and before closing array brackets <FEEDBACK>
+  'array-bracket-newline': ['error', {'multiline': true}],


No need for quotes when names are valid identifiers

Disabling this rule, it's not behaving as expected, or maybe I'm misunderstanding it.

Lint errors show that there should be no linebreak when we specify multiline: true.
https://circleci.com/gh/Shopify/eslint-plugin-shopify/144

lemonmade · 2017-09-18T18:22:41Z

package.json

@@ -58,18 +58,18 @@
    "eslint": "<5 >=4.3.0"


I think we need this to be >=4.7.0 now

ismail-syed · 2017-09-18T20:22:12Z

lib/config/rules/stylistic-issues.js

@@ -1,8 +1,12 @@
 // see http://eslint.org/docs/rules/#stylistic-issues

 module.exports = {
+  // enforce linebreaks after opening and before closing array brackets
+  'array-bracket-newline': 'off',


Disabled this, I misunderstood what the multiline option meant.

Perhaps multiline should be renamed to multiline-element to avoid confusion.
eslint/eslint#8981 (comment)

👍

I kinda like this one, but don't like some of the edge cases that it forbids/allows. Ultimately, I don't see anyone violating this rule so ¯\(ツ)/¯

Boo, what a terrible option :/

ismail-syed · 2017-09-18T20:22:31Z

lib/config/rules/stylistic-issues.js

  // Enforce spacing inside array brackets
  'array-bracket-spacing': ['warn', 'never'],
+  // enforce line breaks after each array element
+  'array-element-newline': 'off',


Disabled, same as https://github.com/Shopify/eslint-plugin-shopify/pull/41/files#r139529700

GoodForOneFare

Couple of minor changes and LGTM. This is a pro PR 👍

GoodForOneFare · 2017-09-19T13:07:55Z

lib/config/rules/import.js

@@ -75,4 +75,6 @@ module.exports = {
  'import/max-dependencies': 'off',
  // Forbid unassigned imports
  'import/no-unassigned-import': 'off',
+  // Forbid anonymous values as default exports
+  'import/no-anonymous-default-export': 'error',


GoodForOneFare · 2017-09-19T13:16:33Z

lib/config/rules/possible-errors.js

+  // enforce “for” loop update clause moving the counter in the right direction.
+  'for-direction': 'error',
+  // enforce return statements in getters
+  'getter-return': ['error', {allowImplicit: true}],


GoodForOneFare · 2017-09-19T13:16:36Z

lib/config/rules/possible-errors.js

@@ -1,6 +1,10 @@
 // see http://eslint.org/docs/rules/#possible-errors

 module.exports = {
+  // enforce “for” loop update clause moving the counter in the right direction.
+  'for-direction': 'error',


GoodForOneFare · 2017-09-19T13:21:18Z

lib/config/rules/react.js

@@ -1,8 +1,12 @@
 module.exports = {
  // General

+  // Enforces consistent naming for boolean props
+  'react/boolean-prop-naming': 'off',


GoodForOneFare · 2017-09-19T13:22:27Z

lib/config/rules/react.js

  // Prevent missing displayName in a React component definition
  'react/display-name': ['warn', {ignoreTranspilerName: false}],
+  // Prevent extraneous defaultProps on components
+  'react/default-props-match-prop-types': 'error',


👍

I assume we'll have to turn this off in the TS config.

GoodForOneFare · 2017-09-19T13:43:14Z

lib/config/rules/stylistic-issues.js

@@ -129,6 +135,8 @@ module.exports = {
  'operator-linebreak': ['warn', 'after', {overrides: {'?': 'before', ':': 'before'}}],
  // Enforce padding within blocks
  'padded-blocks': 'off',
+  // require or disallow padding lines between statements
+  'padding-line-between-statements': 'off',


👍

Yeesh. I'm sure there are useful permutations of this, but who would be able to read the config? 😕

GoodForOneFare · 2017-09-19T13:43:50Z

lib/config/rules/stylistic-issues.js

@@ -137,6 +145,8 @@ module.exports = {
  'require-jsdoc': 'off',
  // Enforce spacing before and after semicolons
  'semi-spacing': ['warn', {before: false, after: true}],
+  // enforce location of semicolons
+  'semi-style': ['error', 'last'],


GoodForOneFare · 2017-09-19T13:44:51Z

lib/config/rules/stylistic-issues.js

@@ -155,6 +165,8 @@ module.exports = {
  'space-unary-ops': ['warn', {words: true, nonwords: false}],
  // Require or disallow a space immediately following the // or /* in a comment
  'spaced-comment': ['warn', 'always', {markers: ['=']}],
+  // enforce spacing around colons of switch statements
+  'switch-colon-spacing': ['error', {'after': true, 'before': false}],


GoodForOneFare · 2017-09-19T13:45:49Z

package.json

@@ -45,7 +45,7 @@
    "babel-cli": "^6.24.1",
    "babel-core": "^6.25.0",
    "babel-preset-shopify": "^16.1.0",
-    "eslint": "^4.3.0",
+    "eslint": "^4.7.0",


GoodForOneFare · 2017-09-19T13:46:05Z

lib/config/rules/jsx-a11y.js

@@ -7,6 +7,8 @@ module.exports = {
  'jsx-a11y/alt-text': 'error',
  // Enforce all anchors to contain accessible content.
  'jsx-a11y/anchor-has-content': 'error',
+  // Enforce all anchors are valid, navigable elements.
+  'jsx-a11y/anchor-is-valid': 'error',


This replaces jsx-a11y/href-no-hash. The old rule's config should be deleted.

Thanks, nice catch!

ismail-syed added 7 commits September 15, 2017 13:47

Update babel-eslint

4a90093

Upgrade eslint-plugin-jsx-a11y

46807c9

:

Upgrade eslint-plugin-node

33cbf41

Upgrade eslint-plugin-react

ca0c260

Upgrade eslint-plugin-import

cefaa6d

Update eslint@4.7.0 & add new rules

9d7f320

Set appropriate rules

825b890

ismail-syed commented Sep 18, 2017

View reviewed changes

ismail-syed requested review from lemonmade and GoodForOneFare September 18, 2017 18:03

lemonmade approved these changes Sep 18, 2017

View reviewed changes

lemonmade reviewed Sep 18, 2017

View reviewed changes

package.json Outdated

@@ -58,18 +58,18 @@

"eslint": "<5 >=4.3.0"

Copy link

Member

lemonmade Sep 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need this to be >=4.7.0 now

ismail-syed added 2 commits September 18, 2017 14:52

PR fixes

f73c113

Updated changelog 📝

8768b84

ismail-syed commented Sep 18, 2017

View reviewed changes

ismail-syed added 2 commits September 18, 2017 16:23

Disable array-bracket-newline & array-element-newline

6c9db39

Update rule option to consistent

48ac1f3

ismail-syed force-pushed the eslint-update branch from 02e8d19 to 48ac1f3 Compare September 18, 2017 20:31

GoodForOneFare approved these changes Sep 19, 2017

View reviewed changes

PR fixes

dfd4d9c

ismail-syed force-pushed the eslint-update branch from ef6eadf to dfd4d9c Compare September 19, 2017 15:30

ismail-syed merged commit 870ec7f into master Sep 19, 2017

ismail-syed deleted the eslint-update branch September 19, 2017 15:31

Upgrade eslint to 4.7.0 & update rules #41

Upgrade eslint to 4.7.0 & update rules #41

Conversation

ismail-syed commented Sep 18, 2017 • edited Loading

ismail-syed Sep 18, 2017 • edited Loading

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

ismail-syed Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemonmade 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

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

ismail-syed Sep 18, 2017 • edited Loading

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

GoodForOneFare 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismail-syed commented Sep 18, 2017 •

edited

Loading

ismail-syed Sep 18, 2017 •

edited

Loading

ismail-syed Sep 18, 2017 •

edited

Loading

ismail-syed Sep 18, 2017 •

edited

Loading