From 38d9b34b2720a2f26e9fa33365836e26d9a7a44e Mon Sep 17 00:00:00 2001 From: Joe Lencioni Date: Tue, 2 Feb 2016 15:30:57 -0800 Subject: [PATCH 1/2] Avoid lexical declarations in case/default clauses This commit adds guidance warning people to avoid lexical declarations in case/default clauses and enables the corresponding ESLint rule. http://eslint.org/docs/rules/no-case-declarations.html We didn't have a section on switch statements yet, so I thought about starting one. It seemed like it would best fit between section 15 (Comparison Operators & Equality) and section 16 (Blocks), but I didn't want to mess up all of the following numberings, since people probably have references to them. I considered adding this near the end to minimize this effect, but it really seemed to belong near these other things. I landed on appending it to Section 15 (Comparison Operators & Equality) and I think it sorta fits there since switch statements are a related concept. --- README.md | 41 +++++++++++++++++++ .../rules/best-practices.js | 2 + 2 files changed, 43 insertions(+) diff --git a/README.md b/README.md index b758421d..381d5c6b 100644 --- a/README.md +++ b/README.md @@ -1245,6 +1245,47 @@ Other Style Guides ``` - [15.4](#15.4) For more information see [Truth Equality and JavaScript](http://javascriptweblog.wordpress.com/2011/02/07/truth-equality-and-javascript/#more-2108) by Angus Croll. + - [15.5](#15.5) Use braces to create blocks in `case` and `default` clauses that contain lexical declarations (e.g. `let`, `const`, `function`, and `class`). + + > Why? Lexical declarations are visible in the entire `switch` block but only get initialized when assigned, which only happens when its `case` is reached. This causes problems when multiple `case` clauses attempt to define the same thing. + + eslint rules: [`no-case-declarations`](http://eslint.org/docs/rules/no-case-declarations.html). + + ```javascript + // bad + switch (foo) { + case 1: + let x = 1; + break; + case 2: + const y = 2; + break; + case 3: + function f() {} + break; + default: + class C {} + } + + // good + switch (foo) { + case 1: { + let x = 1; + break; + } + case 2: { + const y = 2; + break; + } + case 3: { + function f() {} + break; + } + default: { + class C {} + } + } + ``` - [15.5](#15.5) Ternaries should not be nested and generally be single line expressions. diff --git a/packages/eslint-config-airbnb/rules/best-practices.js b/packages/eslint-config-airbnb/rules/best-practices.js index c2af1b78..d8b94e9b 100644 --- a/packages/eslint-config-airbnb/rules/best-practices.js +++ b/packages/eslint-config-airbnb/rules/best-practices.js @@ -24,6 +24,8 @@ module.exports = { 'no-alert': 1, // disallow use of arguments.caller or arguments.callee 'no-caller': 2, + // disallow lexical declarations in case/default clauses + 'no-case-declaration': 2, // disallow division operators explicitly at beginning of regular expression 'no-div-regex': 0, // disallow else after a return in an if From 4d74fe1f5ab7e2f67e4ad837387debbf276e3929 Mon Sep 17 00:00:00 2001 From: Joe Lencioni Date: Wed, 3 Feb 2016 12:23:51 -0800 Subject: [PATCH 2/2] Add example of a case clause that does not need a block As @kesne and @ljharb pointed out, this section was unclear about whether or not you should always include blocks for all case clauses. To make things clearer, I am adding a case clause that does not need a block to the good example. We decided that we should only require blocks for case clauses that actually need them because it matches the as-needed spirit of section 3.8 ("Only quote properties that are invalid identifiers"). Perhaps if there was an as-needed but consistent setting for the ESLint rule, we would consider revising this a little, but this seems good enough for now. --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index 381d5c6b..9920eba7 100644 --- a/README.md +++ b/README.md @@ -1281,6 +1281,9 @@ Other Style Guides function f() {} break; } + case 4: + bar(); + break; default: { class C {} }