Skip to content

Commit f6788e0

Browse files
committed
Update more docs
1 parent 569f02c commit f6788e0

9 files changed

+30
-37
lines changed

docs/rules/array-foreach.md

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,25 +4,17 @@ Prefer `for...of` statement instead of `Array.forEach`.
44

55
## Rule Details
66

7-
### Why disallow `forEach`
8-
97
Here's a summary of why `forEach` is disallowed, and why we prefer `for...of` for almost any use-case of `forEach`:
108

119
- Allowing `forEach` encourages **layering of "bad practices"**, such as using `Array.from()` (which is less performant than using `for...of`).
1210
- When more requirements are added on, `forEach` typically gets **chained** with other methods like `filter` or `map`, causing multiple iterations over the same Array. Encouraging `for` loops discourages chaining and encourages single-iteration logic (e.g. using a `continue` instead of `filter`).
1311
- `for` loops are considered "more readable" and have **clearer intent**.
1412
- `for...of` loops offer the **most flexibility** for iteration (especially vs `Array.from`).
1513

16-
For more detail, here is a breakdown of each of those points:
17-
18-
### Layering of bad practices
19-
2014
Typically developers will reach for a `forEach` when they want to iterate over a set of items. However not all "iterables" have access to Array methods. So a developer might convert their iterable to an Array by using `Array.from(iter).forEach()`. This code has introduced performance problems, where a `for...of` loop would be more performant.
2115

2216
`forEach` does not do anything special with the Array - it does not create a new array or does not aid in encapsulation (except for introducing a new lexical scope within the callback, which isn't a benefit considering we use `let`/`const`). We don't dissallow `map`/`filter`/`reduce` because they have a tangible effect - they create a new array - which would take _more_ code and be _less_ readable to do with a `for...of` loop, the exception being as more requirements are added, and we start chaining array methods together...
2317

24-
### Chaining
25-
2618
Often when using a method like `forEach` - when coming back to add new code, let's say to filter certain elements from the Array before operating on them, a developer is implicitly encouraged to use Array's method chaining to achieve this result. For example if we wanted to filter out bad apples from an Array of Apples, if the code already uses `forEach`, then its a simple addition to add `filter()`:
2719

2820
```diff
@@ -42,8 +34,6 @@ The problem we now have is that we're iterating multiple times over the items in
4234

4335
Chaning isn't always necessarily bad. Chaining can advertise a series of transformations that are independant from one another, and therefore aid readability. Additionally, sometimes the "goto-style" behaviour of `continue` in for loops can hamper readability. For small Arrays, performance is not going to be of concern, but caution should be applied where there is a potentially unbounded Array (such as iterating over a fetched users list) as performance can easily become a bottleneck when unchecked.
4436

45-
### Hiding Intent
46-
4737
The `forEach` method passes more than just the current item it is iterating over. The signature of the `forEach` callback method is `(cur: T, i: Number, all: []T) => void` and it can _additionally_ override the `receiver` (`this` value), meaning that often the _intent_ of what the callback does is hidden. To put this another way, there is _no way_ to know what the following code operates on without reading the implementation: `forEach(polishApple)`.
4838

4939
The `for` loop avoids this issue. Calls are explicit within the `for` loop, as they are not passed around. For example:
@@ -56,8 +46,6 @@ for (const apple of apples) {
5646

5747
We know this code can only possibly mutate `apple`, as the return value is discarded, there is no `receiver` (`this` value) as `.call()` is not used, and it cannot operate on the whole array of `apples` because it is not passed as an argument. In this respect, we can establish what the intent of `polishApple(apple)` is far more than `forEach(polishApple)`. It is too easy for `forEach` to obscure the intent.
5848

59-
### Flexibility
60-
6149
While `forEach` provides a set of arguments to the callback, it is still overall _less flexible_ than a `for` loop. A `for` loop can conditionally call the callback, can pass additional arguments to the callback (which would otherwise need to be hoisted or curried), can opt to change the `receiver` (`this` value) or not pass any `receiver` at all. This extra flexibility is the reason we almost always prefer to use `for` loops over any of the Array iteration methods.
6250

6351
A good example of how `for` loops provide flexibility, where `forEach` constrains it, is to see how an iteration would be refactored to handle async work. Consider the following...
@@ -88,9 +76,7 @@ Compare this to the `for` loop, which has a much simpler path to refactoring:
8876
}
8977
```
9078

91-
### See Also
92-
93-
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
79+
See also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
9480

9581
👎 Examples of **incorrect** code for this rule:
9682

docs/rules/async-currenttarget.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
# Async Currenttarget
22

3+
## Rule Details
4+
35
Accessing `event.currentTarget` inside an `async function()` will likely be `null` as `currentTarget` is mutated as the event is propagated.
46

57
1. A `click` event is dispatched

docs/rules/async-preventdefault.md

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,9 @@ If you're using `async`, you likely need to wait on a promise in the event handl
1616

1717
```js
1818
document.addEventListener('click', async function (event) {
19-
event.preventDefault()
20-
2119
const data = await fetch()
22-
// ...
20+
21+
event.preventDefault()
2322
})
2423
```
2524

docs/rules/authenticity-token.md

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,19 @@ The Rails `form_tag` helper creates a `<form>` element with a `<input name="auth
66

77
An attacker who is able to steal a user's CSRF token can perform a CSRF attack against that user. To reduce this risk, GitHub uses per-form CSRF tokens. This means that a form's method and action are embedded in that form's CSRF token. When the form is submitted, the Rails application verifies that the request's path and method match those of the CSRF token: A stolen token for the `POST /preview` endpoint will not be accepted for the `DELETE /github/github` endpoint.
88

9-
## CSRF tokens in JavaScript
10-
119
Requests initiated by JavaScript using XHR or Fetch still need to include a CSRF token. Prior to our use of per-form tokens, a common pattern for getting a valid CSRF token to include in a request was
1210

11+
Unless the JavaScript's request is for the same method/action as the form from which it takes the CSRF token, this CSRF token will _not_ be accepted by the Rails application.
12+
13+
The preferred way to make an HTTP request with JavaScript is to use the [`FormData`](https://developer.mozilla.org/en-US/docs/Web/API/FormData) API to serialize the input elements of a form:
14+
15+
👎 Examples of **incorrect** code for this rule:
16+
1317
```js
1418
const csrfToken = this.closest('form').elements['authenticity_token'].value
1519
```
1620

17-
Unless the JavaScript's request is for the same method/action as the form from which it takes the CSRF token, this CSRF token will _not_ be accepted by the Rails application.
18-
19-
The preferred way to make an HTTP request with JavaScript is to use the [`FormData`](https://developer.mozilla.org/en-US/docs/Web/API/FormData) API to serialize the input elements of a form:
21+
👍 Examples of **correct** code for this rule:
2022

2123
```erb
2224
<%= form_tag "/my/endpoint" do %>

docs/rules/get-attribute.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,19 @@ As HTML attributes are case insensitive, prefer using lowercase.
88

99
```js
1010
el.getAttribute('autoComplete')
11+
```
12+
13+
```js
1114
el.getAttribute('dataFoo')
1215
```
1316

1417
👍 Examples of **correct** code for this rule:
1518

1619
```js
1720
el.getAttribute('autocomplete')
21+
```
22+
23+
```js
1824
el.getAttribute('data-foo')
1925
```
2026

docs/rules/js-class-name.md

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@ Since its easy for humans to cross reference usage sites and implementation, so
1212

1313
In order to trust this system, all `js-` class names MUST be statically written as string literals. This means no dynamically constructed strings by interpolation. For the same reason, `obj.send("can_#{sym}?")` makes you feel bad deep down inside, so should `querySelector("js-" + sym)`.
1414

15-
### Alternatives
16-
1715
Typically dynamically constructed `js-` classes are often mixing static symbols and user data together. Like `"js-org-#{org.login}"`. In this case, separating into a `data-` attribute would be a better solution.
1816

1917
```html
@@ -22,24 +20,20 @@ Typically dynamically constructed `js-` classes are often mixing static symbols
2220

2321
Allows you to select elements by `js-org-update` and still filter by the `data-org-name` attribute if you need to. Both `js-org-update` and `data-org-name` are clearly static symbols that are easy to search for.
2422

25-
### Formatting
26-
2723
`js-` classes must start with `js-` (obviously) and only contain lowercase letters and numbers separated by `-`s. The ESLint [`github/js-class-name`](https://github.com/github/eslint-plugin-github/blob/master/lib/rules/js-class-name.js) rule enforces this style.
2824

29-
### See Also
30-
3125
[@defunkt's original proposal from 2010](https://web.archive.org/web/20180902223055/http://ozmm.org/posts/slightly_obtrusive_javascript.html).
3226

3327
👎 Examples of **incorrect** code for this rule:
3428

3529
```js
36-
30+
const el = document.querySelector('.js-Foo')
3731
```
3832

3933
👍 Examples of **correct** code for this rule:
4034

4135
```js
42-
36+
const el = document.querySelector('.js-foo')
4337
```
4438

4539
## When Not To Use It

docs/rules/no-dataset.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,13 @@ Due to [camel-case transformations](https://developer.mozilla.org/en-US/docs/Web
77
👎 Examples of **incorrect** code for this rule:
88

99
```js
10-
10+
el.dataset.coolThing
1111
```
1212

1313
👍 Examples of **correct** code for this rule:
1414

1515
```js
16-
16+
el.getAttribute('data-cool-thing')
1717
```
1818

1919
## When Not To Use It

docs/rules/no-implicit-buggy-globals.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,15 @@
55
👎 Examples of **incorrect** code for this rule:
66

77
```js
8-
8+
var foo = 1
99
```
1010

1111
👍 Examples of **correct** code for this rule:
1212

1313
```js
14-
14+
;(function () {
15+
const foo = 1
16+
})()
1517
```
1618

1719
## When Not To Use It

docs/rules/no-innerText.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,15 @@
55
👎 Examples of **incorrect** code for this rule:
66

77
```js
8-
8+
const el = document.createElement('div')
9+
el.innerText = 'foo'
910
```
1011

1112
👍 Examples of **correct** code for this rule:
1213

1314
```js
14-
15+
const el = document.createElement('div')
16+
el.textContent = 'foo'
1517
```
1618

1719
## When Not To Use It

0 commit comments

Comments
 (0)