Use eslint to enforce good const/let/var usage #113
Conversation
Motivation: Currently the guidelines put this into the "linting won't catch this" section, because we've just been using plain semistandard. Modification: * Add prefer-const rule, which will force usage of const if an identifier is never reassigned. More info: http://eslint.org/docs/rules/prefer-const * Add no-use-before-define rule, which will prevent ReferenceErrors being thrown due to "temporal dead zone" issues with const/let. More info: http://eslint.org/docs/rules/no-use-before-define * Add block-scoped-var rule, which will force var to act like let. The current guidelines recommend against using var, and instead using let for mutable variables. Unfortunately, let currently doesn't perform as well as const or var: nodejs/node#8637 (comment) More info on this rule: http://eslint.org/docs/rules/block-scoped-var * Remove the recommendation to check for this manually.
|
@lance @lholmquist Please take a look if you're interested, thanks! |
| @@ -154,12 +160,9 @@ At this point, we eschew any Node.js version below 4.x. This means, that usage o | |||
| features is possible. And there are some good features to take advatage of. Here are | |||
| some of our recommendations. | |||
|
|
|||
| * Use `let` and `const` instead of `var`. There is rarely ever a valid case for using | |||
| `var` in modern Javascript code. If the variable is immutable (which we believe most | |||
| should be), use `const`. If the variable may be written, use `let`. | |||
There was a problem hiding this comment.
Looks good overall. I wonder if there is any value to leaving this text in the document some place. I realize it's not relevant in the "what linting won't catch" section any longer. Maybe just as a comment in the rules above?
There was a problem hiding this comment.
I've added a reworded version of that to where I think you're talking about. Additionally:
Maybe just as a comment in the rules above?
This caught my attention for a different reason: have you considered having the eslint config as yaml instead of json? It's a supported format, but I haven't tried it, so I'm not sure of disadvantages. One advantage would be "comments in the rules"! :)
|
@grdryn lgtm |
|
👍 |
| including dependencies, if the other code is not aware of the modifications. Using `const` | ||
| is preferred to `var`. If a variable must be mutable, use `let` (`var` is also allowed for | ||
| mutable variables for cases where `let` may cause performance issues, but is restricted to | ||
| block scope, so will act like `let`. For these and many other reasons, we think linting is a |
There was a problem hiding this comment.
Hrmm, so this diff ain't the best, is it? I set my editor to what I determined to be the max line length in the paragraph previously (92? lol my emacs wanted it to be 70), so when I put new content in the middle, the entire paragraph got re-flowed.
One possible improvement that could be considered to lead to better diffs in your markdown (or other content files), could be to start each sentence on a new line (of course you can line-break within a long sentence also). As long as there isn't a blank line between two sentences in markdown, they should be rendered as a single paragraph (i.e. the fact that a new sentence started on a new sentence in the raw version of the file doesn't matter: it'll be re-flowed to be a unified paragraph in the rendered/rich version).
Maybe I'm thinking about this too much...we probably don't need a .mdlint.json to talk about .eslintrc.json ;)
There was a problem hiding this comment.
Yeah, docs are hard. I think I would prefer a standard line length (that's not 92 :).
| @@ -112,7 +112,13 @@ plugins for `eslint` in our builds. | |||
| First, add an `.eslintrc.json` file to your project and put this in there. | |||
|
|
|||
| { | |||
There was a problem hiding this comment.
Like I mentioned in the previous diff here, this could maybe become .eslint.yaml, in which case it could contain comments(!) and could look something like this:
extends: semistandard
rules:
prefer-const: error
block-scoped-var: error
no-use-before-define:
- error
- nofuncOr with copious amounts of comments:
extends: semistandard
rules:
# Error if a let/var is never mutated -- must be changed to const
prefer-const: error
# Make var act like let
block-scoped-var: error
# Prevent ReferenceError due to "temporal dead zone"
# For more info, see: http://eslint.org/docs/rules/no-use-before-define
no-use-before-define:
- error
- nofunc # Ignore functions, as they're always hoisted
|
@grdryn thanks for the contribution! |
Motivation:
Currently the guidelines put this into the "linting won't catch this" section, because we've just been using plain semistandard.
Modification:
Add prefer-const rule, which will force usage of const if an identifier is never reassigned. More info: http://eslint.org/docs/rules/prefer-const
Add no-use-before-define rule, which will prevent ReferenceErrors being thrown due to "temporal dead zone" issues with const/let. More info: http://eslint.org/docs/rules/no-use-before-define
Add block-scoped-var rule, which will force var to act like let. The current guidelines recommend against using var, and instead using let for mutable variables. Unfortunately, let currently doesn't perform as well as const or var:
Benchmark var -> const before merging mass rewrites nodejs/node#8637 (comment)
More info on this rule: http://eslint.org/docs/rules/block-scoped-var
Remove the recommendation to check for this manually.
Note: If you're happy with the first two rules, but would prefer to completely disallow var, even with the current performance issues; I can modify this commit to use the no-var rule instead of the block-scoped-var rule. Let me know what you think! :)