denoland / deno_lint

Blazing fast linter for JavaScript and TypeScript written in Rust
https://lint.deno.land/
MIT License
1.55k stars 172 forks source link

additional optional rules to consider #906

Open traceypooh opened 3 years ago

traceypooh commented 3 years ago

from https://github.com/denoland/deno_lint/issues/176

some eslint + airbnb base rules that could be nice to have w/ deno lint:

guard-for-in
import/no-extraneous-dependencies
import/no-named-as-default
import/no-named-as-default-member
import/no-unresolved
import/prefer-default-export
max-len
new-cap
no-alert
no-cond-assign
no-console
no-continue
no-multi-assign
no-nested-ternary
no-param-reassign
no-plusplus
no-return-assign
no-return-await
no-shadow
no-tabs
no-underscore-dangle
no-use-before-define
no-unused-expressions
no-useless-concat
no-useless-escape
prefer-destructuring
prefer-rest-params
quotes
semi

some of these I could live without -- but for example quotes and no-useless-concat me and team are heavily interested in and invested in. no-plusplus is nice esp. if you're not coding with semis -- where semi is hugely useful (to flag ; where you're about to footgun - for example code right in front of a IIFE, etc.

traceypooh commented 3 years ago

in case useful, here's a sorted list of all the rules from doing a eslint --print-config

based upon this config:

{
  "extends": "airbnb-base",
  "plugins": [
    "compat"
  ],
  "parserOptions": {
    "sourceType": "module",
    "ecmaVersion": 2020
  },
  "env": {
    "browser": true,
    "jasmine": true
  },
  "globals": {
    "Deno": false
  },
  "ignorePatterns": [
    "**/*.min.js",
    "node_modules/"
  ],
  "rules": {
    // allow `import .. from '.js'` (.js suffix) in JS files
    "import/extensions": ["off"],

    // this just showed up as necessary w/ `npm i` on Jun11, 2020
    "no-multiple-empty-lines": [2, {"max": 2}],

    // this just showed up w/ babel + eslint updates to latest versions Sep1,2019
    "operator-linebreak": "off",
    "import/no-cycle": "off", // it's ok to have cycles with ES Modules and import

    // "make sure all used JS compatible with 90%+ of currently used browsers a la caniuse.com"
    "compat/compat": "error",

    // "allow snakecase var names if dev desires"
    "camelcase": "off",

    // "allow: x  = 3 (for example lining up multiple lines by column)"
    "no-multi-spaces": "off",

    // "author discretion when using braces around one-liners or same-liners"
    "curly": "off",

    // "allow ++ or -- at the end fo a for() loop (all other uses are banned per airbnb!)"
    "no-plusplus": ["error", {"allowForLoopAfterthoughts": true}],

    // "allow JSON/map definitions to column-align values when multiline"
    "key-spacing": ["error", {"mode": "minimum"}],

    // "allow for (x of array)  and  for (key in obj)  and   for (val in array)"
    "no-restricted-syntax": ["error", "LabeledStatement", "WithStatement"],

    "no-restricted-globals": ["off", "location"],

    "nonblock-statement-body-position": "off",

    "indent": ["error", 2, {"CallExpression": {"arguments": "first"},
                            "ArrayExpression": "first",
                            "FunctionDeclaration": {"parameters": "first"},
                            "FunctionExpression": {"body": 1, "parameters": 2} }],

    "semi": ["error", "never", { "beforeStatementContinuationChars": "always" }]
  }
}
accessor-pairs
array-bracket-newline
array-bracket-spacing
array-callback-return
array-element-newline
arrow-body-style
arrow-parens
arrow-spacing
block-scoped-var
block-spacing
brace-style
callback-return
camelcase
capitalized-comments
class-methods-use-this
comma-dangle
comma-spacing
comma-style
compat/compat
complexity
computed-property-spacing
consistent-return
consistent-this
constructor-super
curly
default-case
default-case-last
default-param-last
dot-location
dot-notation
eol-last
eqeqeq
for-direction
func-call-spacing
func-name-matching
func-names
func-style
function-call-argument-newline
function-paren-newline
generator-star-spacing
getter-return
global-require
grouped-accessor-pairs
guard-for-in
handle-callback-err
id-blacklist
id-length
id-match
implicit-arrow-linebreak
import/default
import/dynamic-import-chunkname
import/export
import/exports-last
import/extensions
import/first
import/group-exports
import/imports-first
import/max-dependencies
import/named
import/namespace
import/newline-after-import
import/no-absolute-path
import/no-amd
import/no-anonymous-default-export
import/no-commonjs
import/no-cycle
import/no-default-export
import/no-deprecated
import/no-duplicates
import/no-dynamic-require
import/no-extraneous-dependencies
import/no-internal-modules
import/no-mutable-exports
import/no-named-as-default
import/no-named-as-default-member
import/no-named-default
import/no-named-export
import/no-namespace
import/no-nodejs-modules
import/no-relative-parent-imports
import/no-restricted-paths
import/no-self-import
import/no-unassigned-import
import/no-unresolved
import/no-unused-modules
import/no-useless-path-segments
import/no-webpack-loader-syntax
import/order
import/prefer-default-export
import/unambiguous
indent
init-declarations
jsx-quotes
key-spacing
keyword-spacing
line-comment-position
linebreak-style
lines-around-comment
lines-around-directive
lines-between-class-members
max-classes-per-file
max-depth
max-len
max-lines
max-lines-per-function
max-nested-callbacks
max-params
max-statements
max-statements-per-line
multiline-comment-style
multiline-ternary
new-cap
new-parens
newline-after-var
newline-before-return
newline-per-chained-call
no-alert
no-array-constructor
no-async-promise-executor
no-await-in-loop
no-bitwise
no-buffer-constructor
no-caller
no-case-declarations
no-catch-shadow
no-class-assign
no-compare-neg-zero
no-cond-assign
no-confusing-arrow
no-console
no-const-assign
no-constant-condition
no-constructor-return
no-continue
no-control-regex
no-debugger
no-delete-var
no-div-regex
no-dupe-args
no-dupe-class-members
no-dupe-else-if
no-dupe-keys
no-duplicate-case
no-duplicate-imports
no-else-return
no-empty
no-empty-character-class
no-empty-function
no-empty-pattern
no-eq-null
no-eval
no-ex-assign
no-extend-native
no-extra-bind
no-extra-boolean-cast
no-extra-label
no-extra-parens
no-extra-semi
no-fallthrough
no-floating-decimal
no-func-assign
no-global-assign
no-implicit-coercion
no-implicit-globals
no-implied-eval
no-import-assign
no-inline-comments
no-inner-declarations
no-invalid-regexp
no-invalid-this
no-irregular-whitespace
no-iterator
no-label-var
no-labels
no-lone-blocks
no-lonely-if
no-loop-func
no-loss-of-precision
no-magic-numbers
no-misleading-character-class
no-mixed-operators
no-mixed-requires
no-mixed-spaces-and-tabs
no-multi-assign
no-multi-spaces
no-multi-str
no-multiple-empty-lines
no-native-reassign
no-negated-condition
no-negated-in-lhs
no-nested-ternary
no-new
no-new-func
no-new-object
no-new-require
no-new-symbol
no-new-wrappers
no-obj-calls
no-octal
no-octal-escape
no-param-reassign
no-path-concat
no-plusplus
no-process-env
no-process-exit
no-proto
no-prototype-builtins
no-redeclare
no-regex-spaces
no-restricted-exports
no-restricted-globals
no-restricted-imports
no-restricted-modules
no-restricted-properties
no-restricted-syntax
no-return-assign
no-return-await
no-script-url
no-self-assign
no-self-compare
no-sequences
no-setter-return
no-shadow
no-shadow-restricted-names
no-spaced-func
no-sparse-arrays
no-sync
no-tabs
no-template-curly-in-string
no-ternary
no-this-before-super
no-throw-literal
no-trailing-spaces
no-undef
no-undef-init
no-undefined
no-underscore-dangle
no-unexpected-multiline
no-unmodified-loop-condition
no-unneeded-ternary
no-unreachable
no-unsafe-finally
no-unsafe-negation
no-unused-expressions
no-unused-labels
no-unused-vars
no-use-before-define
no-useless-backreference
no-useless-call
no-useless-catch
no-useless-computed-key
no-useless-concat
no-useless-constructor
no-useless-escape
no-useless-rename
no-useless-return
no-var
no-void
no-warning-comments
no-whitespace-before-property
no-with
nonblock-statement-body-position
object-curly-newline
object-curly-spacing
object-property-newline
object-shorthand
one-var
one-var-declaration-per-line
operator-assignment
operator-linebreak
padded-blocks
padding-line-between-statements
prefer-arrow-callback
prefer-const
prefer-destructuring
prefer-exponentiation-operator
prefer-named-capture-group
prefer-numeric-literals
prefer-object-spread
prefer-promise-reject-errors
prefer-reflect
prefer-regex-literals
prefer-rest-params
prefer-spread
prefer-template
quote-props
quotes
radix
require-atomic-updates
require-await
require-jsdoc
require-unicode-regexp
require-yield
rest-spread-spacing
semi
semi-spacing
semi-style
sort-imports
sort-keys
sort-vars
space-before-blocks
space-before-function-paren
space-in-parens
space-infix-ops
space-unary-ops
spaced-comment
strict
switch-colon-spacing
symbol-description
template-curly-spacing
template-tag-spacing
unicode-bom
use-isnan
valid-jsdoc
valid-typeof
vars-on-top
wrap-iife
wrap-regex
yield-star-spacing
yoda
traceypooh commented 3 years ago

1000 apologies.

after git clone of the repo, i realized it looks like each rule is a manually written/maintained parser file (in my naive mind, I imagined majority might be turning off => on certain eslint rules...) :(

so... over a coupla beers... i went over the 320 rules enabled w/ my (mostly default eslint/airbnb) setup; compared against the 68 deno rules, leaving ~266 rules to pore over).

here's my shorter list of (uh) ~78 rules that me + team have found [legit] useful over last few years:

class-methods-use-this
comma-spacing
compat/compat
consistent-return
curly
dot-location
dot-notation
eol-last
eqeqeq
func-call-spacing
function-call-argument-newline
import/no-unresolved
indent
key-spacing
keyword-spacing
linebreak-style
lines-between-class-members
max-classes-per-file
max-len
multiline-comment-style
new-parens
no-await-in-loop
no-bitwise
no-cond-assign
no-console
no-const-assign
no-constructor-return
no-div-regex
no-duplicate-imports
no-else-return
no-eq-null
no-extend-native
no-implicit-coercion
no-invalid-this
no-lonely-if
no-mixed-operators
no-multi-assign
no-multi-str
no-multiple-empty-lines
no-native-reassign
no-negated-in-lhs
no-new-wrappers
no-octal-escape
no-param-reassign
no-plusplus
no-return-assign
no-script-url
no-shadow
no-sparse-arrays
no-tabs
no-template-curly-in-string
no-throw-literal
no-trailing-spaces
no-undef
no-unexpected-multiline
no-unneeded-ternary
no-unused-expressions
no-use-before-define
no-useless-computed-key
no-useless-concat
no-useless-escape
no-var
no-void
no-whitespace-before-property
object-curly-spacing
one-var
operator-assignment
padded-blocks
prefer-arrow-callback
prefer-object-spread
prefer-rest-params
prefer-template
quotes
radix
semi
space-before-blocks
space-infix-ops
space-unary-ops
spaced-comment
wrap-iife
bartlomieju commented 3 years ago

@traceypooh we're discussing internally providing compatibility with ESLint and ability to load ESLint plugins - this should solve your needs more generally. We could try and tackle the list you provided by implementing these rules in Rust, but there's many more plugins for specific frontend frameworks too, and I don't see us implementing all possible rules.

traceypooh commented 3 years ago

OMG, that would be so sooo amazing.
I've migrated a 30,000 line JS project (mostly backend A/V + TV processing) from node to deno and the only thing I need npm w/ minimal package.json for now is during CI [lint] phase of our pipelines, where I:

So it'd be so sooo nice to get the impossibly fast deno lint setup and dump eslint and npm + package.json altogether!

:)

bartlomieju commented 2 years ago

@traceypooh just FYI - you can now run eslint natively in Deno using "Node compatbility mode", see https://deno.land/manual@main/npm_nodejs/compatibility_mode for more information. Hope it helps in the meantime until deno_lint can consume ESLint's plugins.

traceypooh commented 2 years ago

ok, this is a very interesting middle ground - thanks for the add and pointer!

apologies for delay in replying -- i hadn't had the time to really drill down to figure out why my setup wasn't working.

so the good news is that i have my full normal eslint setup working (w/ extends: 'airbnb-base', the compat plugin, and lots more.

the bad news is what doesn't work (and fatals running eslint for me) is any deno import, eg: import { existsSync } from 'https://deno.land/std/fs/mod.ts'

will cause this to happen:

deno run --compat --unstable --allow-read --allow-write=./ --allow-env ~/node_modules/.bin/eslint *.js
Not implemented: process.on("uncaughtException")
Not implemented: process.on("unhandledRejection")

Oops! Something went wrong! :(

ESLint: 7.32.0

TypeError: Path must be a string. Received null
Occurred while linting /Users/tracey/dev/serverless/index.js:3
    at assertPath (https://deno.land/std@0.116.0/path/_util.ts:18:11)
    at dirname (https://deno.land/std@0.116.0/path/posix.ts:222:3)
    at getFilePackagePath (file:///Users/tracey/node_modules/eslint-plugin-import/lib/core/packagePath.js:15:641)
    at getContextPackagePath (file:///Users/tracey/node_modules/eslint-plugin-import/lib/core/packagePath.js:15:420)
    at typeTest (file:///Users/tracey/node_modules/eslint-plugin-import/lib/core/importType.js:105:2296)
    at resolveImportType (file:///Users/tracey/node_modules/eslint-plugin-import/lib/core/importType.js:105:2640)
    at reportIfMissing (file:///Users/tracey/node_modules/eslint-plugin-import/lib/rules/no-extraneous-dependencies.js:170:35)
    at commonjs (file:///Users/tracey/node_modules/eslint-plugin-import/lib/rules/no-extraneous-dependencies.js:267:9)
    at checkSourceValue (file:///Users/tracey/node_modules/eslint-module-utils/moduleVisitor.js:29:5)
    at checkSource (file:///Users/tracey/node_modules/eslint-module-utils/moduleVisitor.js:34:5)

it doesn't matter if i reduce eslint all the way down to just it (ie: no plugins, no custom rules, the absolute barest of .eslintrc.cjs, etc.

As a simple way to show this, one can try in an empty directory:

echo '{ "dependencies": { "eslint": "" } }' > package.json
npm i
echo "import { existsSync } from 'https://deno.land/std/fs/mod.ts'\n x = 3" > index.js
deno run --compat --unstable --allow-read --allow-write=./ --allow-env node_modules/.bin/eslint index.js

And you should see:

...
TypeError: Path must be a string. Received null
...

I eventually discovered this by hopefully setting up an eslint docker base image, and starting to convert a small project from node to deno. To my surprise, the deno compat eslint worked with the node-only code I had, so I continued w/ the conversion, got that all working, and then started to go backwards and figure out why the deno ... eslint wasn't working anymore.

Any thoughts? I'm wondering if there's anything that could be worked out w/ the deno and eslint projects? (I saw another thread where a lead from eslint was interested in helping out being more compatible with deno... :) )

(Lastly, if I fully comment out the import line in JS, then deno ... eslint works fine (w/ the exception that a lot of methods are now "undefined". It didn't suffice to add a blanket eslint-disable-next-line before each import, etc.)

bartlomieju commented 2 years ago

I believe this might be caused by the fact that eslint doesn't support remote imports (although I'd expect it does, eg. in web projects). I'm a bit out of my depth here, and would need to do some research. @nzakas sorry for the ping, but maybe you could clear it up?

nzakas commented 2 years ago

ESLint doesn’t interpret import statements in source code, it just sees an AST.

From my limited experience with Deno, it seems more likely that Deno is implementing something in a typesafe way that Node.js is not. I’m not sure what eslint-module-utils

You can run ESLint with —debug to see if that gets you any closer to finding the culprit.

If you can figure out what’s breaking, and if it’s in ESLint itself, please open an issue and we can take a look.

bartlomieju commented 2 years ago

@traceypooh I did some debugging and the issues seem to stem from the fact that eslint-plugin-import expects all specifiers to refer to either local files or packages in node_modules, or in other words it doesn't expect remote URLs.

In the concrete error you get context that is passed along, returns null from one of the functions called here here: https://github.com/import-js/eslint-plugin-import/blob/e15631696440396f39dad6daf99d48f56f7defce/src/core/packagePath.js#L7

It's not clear to me what's the best cource of action is - on one hand eslint-plugin-import would need to understand the remote URLs, but on the other it would have to know it's run in Deno context and looks for that file in Deno's cache.

traceypooh commented 2 years ago

awesome, thanks muchly for the digging @nzakas and @bartlomieju

this looks potentially interesting as perhaps a way to dodge? https://github.com/import-js/eslint-plugin-import#resolvers

perhaps we could write a little deno-friendly resolver that allows for local files and remote ones starting with https:// (not sure about following on to that to have it be able to parse the remote file and/or look in the deno cache)?

bartlomieju commented 2 years ago

awesome, thanks muchly for the digging @nzakas and @bartlomieju

this looks potentially interesting as perhaps a way to dodge? https://github.com/import-js/eslint-plugin-import#resolvers

perhaps we could write a little deno-friendly resolver that allows for local files and remote ones starting with https:// (not sure about following on to that to have it be able to parse the remote file and/or look in the deno cache)?

Yes, writing a custom resolver would be the way to go; that said, writing resolver is easy, it's way harder to write tools that would check in Deno cache if the file exists. @kitsonk was making strides toward providing a standalone crate/JS API that could integrate with Deno's cache, but AFAIK it is not available yet.

traceypooh commented 2 years ago

So for now at least, since most anything we care about at archive.org is running in docker containers these days, and mostly using GitLab CI/CD, I'm switching tactics.

here's my linter image setup & script: https://gitlab.com/internetarchive/eslint https://gitlab.com/internetarchive/eslint/-/blob/main/lint

here's an example repo (fun lil' game :-) that uses it: https://gitlab.com/internetarchive/word-salad https://gitlab.com/internetarchive/word-salad/-/blob/main/.gitlab-ci.yml#L7

When I'm doing development, all my repos are in my unix or mac $HOME dir -- so I have the same package.json as the [eslint] docker image has, at $HOME/package.json. Doing npm i there allows any repo (in a subdir somewhere to) to "find" that and eslint and deno lint the same way.

Very nice to completely cut out the remaining "weight" of node/npm in my projects. And they all get a great set of linting with just 5 easy .gitlab-ci.yml lines.

Figured I might share, in case anyone else is in same boat, or wants to do something similar with github and github actions