Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eslint rules #90

Closed
mifi opened this issue Jan 17, 2021 · 8 comments
Closed

eslint rules #90

mifi opened this issue Jan 17, 2021 · 8 comments
Assignees
Labels
enhancement sdks Integrations for Transloadit's API

Comments

@mifi
Copy link
Collaborator

mifi commented Jan 17, 2021

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
Copy link
Collaborator Author

mifi commented Jan 17, 2021

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
Copy link
Member

kvz commented Jan 18, 2021

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.

@lekevbot lekevbot added the sdks Integrations for Transloadit's API label Feb 4, 2021
@kvz
Copy link
Member

kvz commented Feb 4, 2021

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
Copy link
Collaborator Author

mifi commented Feb 4, 2021

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
Copy link
Member

kvz commented Feb 5, 2021

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
Copy link
Collaborator Author

mifi commented Feb 5, 2021

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
Copy link
Member

kvz commented Feb 5, 2021

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. (?)

@mifi
Copy link
Collaborator Author

mifi commented Feb 17, 2021

@mifi mifi closed this as completed Feb 17, 2021
kvz pushed a commit that referenced this issue Feb 17, 2021
* extend eslint airbnb and refactor to respect rules

* fix remaining lint issues

* fix buggy tests

* Add a test for totalBytes in uploadProgress

(not yet implemented for isResumable=false)

* Move out eslint #90

https://github.com/transloadit/eslint-config-transloadit

* upgrade to newest eslint-config-transloadit version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement sdks Integrations for Transloadit's API
Projects
None yet
Development

No branches or pull requests

3 participants