transloadit/node-sdk

eslint rules

Closed this issue · 8 comments

mifi commented

I wrote a little script that compares rules from eslint-config-airbnb-base and eslint-config-standard.

Results

Rules in standard but not in airbnb

(Excluding style related rules)

Many of these are node related rules (airbnb does not include any node rules)
Regardless of which style we are using, we may want to include more of the [Node rules].(https://github.com/mysticatea/eslint-plugin-node)
Most of the others are already in airbnb but not yet enabled, until eslint v7 is required (airbnb currently supports eslint v5, v6 or v7 now)

{
  'accessor-pairs': [ 'error', { setWithoutGet: true, enforceForClassMembers: true } ],
  'default-case-last': 'error',
  'no-useless-backreference': 'error',
  'no-extra-parens': [ 'error', 'functions' ],
  'no-import-assign': 'error',
  'no-loss-of-precision': 'error',
  'no-unmodified-loop-condition': 'error',
  'no-unreachable-loop': 'error',
  'no-useless-call': 'error',
  'prefer-regex-literals': [ 'error', { disallowRedundantWrapping: true } ],
  'node/handle-callback-err': [ 'error', '^(err|error)$' ],
  'node/no-callback-literal': 'error',
  'node/no-deprecated-api': 'error',
  'node/no-exports-assign': 'error',
  'node/no-new-require': 'error',
  'node/no-path-concat': 'error',
  'node/process-exit-as-throw': 'error',
  'promise/param-names': 'error'
}

Rules in airbnb but not in standard

(Excluding style related rules)

I tried to search for many of these rules in standard github issues, but I found no mentions.

{
  'block-scoped-var': 'error',
  'class-methods-use-this': [ 'error', { exceptMethods: [] } ],
  'consistent-return': 'error',
  'default-case': [ 'error', { commentPattern: '^no default$' } ],
  'guard-for-in': 'error',
  'max-classes-per-file': [ 'error', 1 ],
  'no-alert': 'warn',
  'no-else-return': [ 'error', { allowElseIf: false } ],
  'no-empty-function': [ 'error', { allow: [Array] } ],
  'no-extra-label': 'error',
  'no-loop-func': 'error',
  'no-param-reassign': [ 'error', { props: true, ignorePropertyModificationsFor: [Array] } ],
  'no-restricted-properties': [
    'error',
    {
      object: 'arguments',
      property: 'callee',
      message: 'arguments.callee is deprecated'
    },
    {
      object: 'global',
      property: 'isFinite',
      message: 'Please use Number.isFinite instead'
    },
    {
      object: 'self',
      property: 'isFinite',
      message: 'Please use Number.isFinite instead'
    },
    {
      object: 'window',
      property: 'isFinite',
      message: 'Please use Number.isFinite instead'
    },
    {
      object: 'global',
      property: 'isNaN',
      message: 'Please use Number.isNaN instead'
    },
    {
      object: 'self',
      property: 'isNaN',
      message: 'Please use Number.isNaN instead'
    },
    {
      object: 'window',
      property: 'isNaN',
      message: 'Please use Number.isNaN instead'
    },
    {
      property: '__defineGetter__',
      message: 'Please use Object.defineProperty instead.'
    },
    {
      property: '__defineSetter__',
      message: 'Please use Object.defineProperty instead.'
    },
    {
      object: 'Math',
      property: 'pow',
      message: 'Use the exponentiation operator (**) instead.'
    }
  ],
  'no-return-await': 'error',
  'no-script-url': 'error',
  'no-unused-labels': 'error',
  'no-useless-concat': 'error',
  radix: 'error',
  'vars-on-top': 'error',
  'for-direction': 'error',
  'getter-return': [ 'error', { allowImplicit: true } ],
  'no-await-in-loop': 'error',
  'no-console': 'warn',
  'no-extra-semi': 'error',
  'no-inner-declarations': 'error',
  'global-require': 'error',
  'no-buffer-constructor': 'error',
  'no-new-require': 'error',
  'no-path-concat': 'error',
  'no-label-var': 'error',
  'no-restricted-globals': [
    'error',
    {
      name: 'isFinite',
      message: 'Use Number.isFinite instead https://github.com/airbnb/javascript#standard-library--isfinite'
    },
    {
      name: 'isNaN',
      message: 'Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan'
    },
    'addEventListener',
    'blur',
    'close',
    'closed',
    'confirm',
    'defaultStatus',
    'defaultstatus',
    'event',
    'external',
    'find',
    'focus',
    'frameElement',
    'frames',
    'history',
    'innerHeight',
    'innerWidth',
    'length',
    'location',
    'locationbar',
    'menubar',
    'moveBy',
    'moveTo',
    'name',
    'onblur',
    'onerror',
    'onfocus',
    'onload',
    'onresize',
    'onunload',
    'open',
    'opener',
    'opera',
    'outerHeight',
    'outerWidth',
    'pageXOffset',
    'pageYOffset',
    'parent',
    'print',
    'removeEventListener',
    'resizeBy',
    'resizeTo',
    'screen',
    'screenLeft',
    'screenTop',
    'screenX',
    'screenY',
    'scroll',
    'scrollbars',
    'scrollBy',
    'scrollTo',
    'scrollX',
    'scrollY',
    'self',
    'status',
    'statusbar',
    'stop',
    'toolbar',
    'top'
  ],
  'no-shadow': 'error',
  'arrow-body-style': [ 'error', 'as-needed', { requireReturnForObjectLiteral: false } ],
  'arrow-parens': [ 'error', 'always' ],
  'no-confusing-arrow': [ 'error', { allowParens: true } ],
  'object-shorthand': [
    'error',
    'always',
    { ignoreConstructors: false, avoidQuotes: true }
  ],
  'prefer-arrow-callback': [ 'error', { allowNamedFunctions: false, allowUnboundThis: true } ],
  'prefer-destructuring': [
    'error',
    { VariableDeclarator: [Object], AssignmentExpression: [Object] },
    { enforceForRenamedProperties: false }
  ],
  'prefer-numeric-literals': 'error',
  'prefer-rest-params': 'error',
  'prefer-spread': 'error',
  'prefer-template': 'error',
  'require-yield': 'error',
  'import/no-unresolved': [ 'error', { commonjs: true, caseSensitive: true } ],
  'import/named': 'error',
  'import/no-named-as-default': 'error',
  'import/no-named-as-default-member': 'error',
  'import/no-extraneous-dependencies': [
    'error',
    { devDependencies: [Array], optionalDependencies: false }
  ],
  'import/no-mutable-exports': 'error',
  'import/no-amd': 'error',
  'import/extensions': [
    'error',
    'ignorePackages',
    { js: 'never', mjs: 'never', jsx: 'never' }
  ],
  'import/order': [ 'error', { groups: [Array] } ],
  'import/newline-after-import': 'error',
  'import/prefer-default-export': 'error',
  'import/no-dynamic-require': 'error',
  'import/no-self-import': 'error',
  'import/no-cycle': [ 'error', { maxDepth: '∞' } ],
  'import/no-useless-path-segments': [ 'error', { commonjs: true } ],
  strict: [ 'error', 'never' ]
}

Other

I also noticed the rule key-spacing is causing big commit diffs if just one line is changed.

'key-spacing': [

mifi commented

here's the script

yarn add eslint-config-standard eslint-config-airbnb-base
const airbnbBases = [
  'rules/best-practices',
  'rules/errors',
  'rules/node',
  'rules/style',
  'rules/variables',
  'rules/es6',
  'rules/imports',
  'rules/strict',
]

// Allow filtering rules that are not enabled
const isEnabled = (val) => val !== 'off' && val[0] !== 'off'

const airbnbMap = Object.fromEntries(airbnbBases.map((extend) => [extend, require(`eslint-config-airbnb-base/${extend}`).rules]))

const airbnb = Object.values(airbnbMap).reduce((acc, v) => ({ ...acc, ...v }))

const airbnbStyleRules = airbnbMap['rules/style']
// console.log(airbnbStyleRules)

const airbnbEnabled = Object.fromEntries(Object.entries(airbnb).filter(([key, val]) => isEnabled(val)))

const standard = require('eslint-config-standard').rules

const standardEnabled = Object.fromEntries(Object.entries(standard).filter(([key, val]) => isEnabled(val)))

const diff1 = {}
Object.entries(airbnbEnabled).forEach(([key, val]) => {
  // Exclude any style related rules
  if (!standardEnabled[key] && !airbnbStyleRules[key]) diff1[key] = val
})

const diff2 = {}
Object.entries(standardEnabled).forEach(([key, val]) => {
  // Exclude any style related rules
  if (!airbnbEnabled[key] && !airbnbStyleRules[key]) diff2[key] = val
})

console.log('Rules in airbnb but not in standard', diff1)

console.log('Rules in standard but not in airbnb', diff2)
kvz commented

If there are rules missing from standard that are particularly dear to you, shall we add them to to our configs on top of standardjs? Would it make sense to publish @transloadit/standard so that we can re-use this across repos? We have https://github.com/transloadit/monolib/ for easily publishing small functions into @transloadit, but like for this project a separate repo makes more sense - if at all.

kvz commented

Different idea, we could also fork the airbnb standard as Transloadit's, then modify it until output results in practically no diff when we autofix our projects with it 🤔

mifi commented

Possible but then we woukld need to continuously rebase/merge our fork from origin which could cause conflicts all the time due to us having modified most rules (especially the style rules)

kvz commented

I said fork but isn’t it possible to use their preset and then lay standard and your own rules on top in eslint? And could that bundling become our own preset? Then updating is a matter of updating npms inside our own style module?

mifi commented

We can create our own config and extend eslint-airbnb-base and selectively disable rules from airbnb base.

As for laying standard on top of airbnb, I'm not sure if that's the best idea as they are both very opinionated rulesets which change all the time. They have many of the same rules enabled but with different configs, and I don't know if in eslint one rule can conflict with another rule.

So we can either:

  • Try merging them and see if it works
  • extend from standard and include extra rules from airbnb base (and our own)
  • extend from airbnb base and include/override extra rules that we want from standard (and our own)
kvz commented

extend from airbnb base and include/override extra rules that we want from standard (and our own)

Given how big airbnb was, i thought something like this may be the smaller task. If we can catch all errors that airbnb would, but keep the style that we have adopted in our projects, that would be a win.

Can try for a few hours and if this turns into a spaceship, we can abort. (?)