-
Notifications
You must be signed in to change notification settings - Fork 61
Add event listener lints #110
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
Changes from 4 commits
c24afd0
85d8b08
6fd9570
1d3c50c
c2d9be4
357c8ec
2c5cda0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# No Non Passive High Frequency Events | ||
|
||
This rule disallows adding high frequency event listeners (`touchstart`, `touchmove`, `wheel`, `mousewheel`) without the `passive: true` option. | ||
|
||
```js | ||
// bad | ||
window.addEventListener('touchstart', () => { /* ... */ }) | ||
|
||
// good | ||
window.addEventListener('touchstart', () => { /* ... */ }, { passive: true }) | ||
``` | ||
|
||
## Why? | ||
|
||
Adding these events listeners can block the main thread as it waits to find out if the callbacks call `preventDefault`. This can cause large amounts UI lag, which will be noticeable for users. | ||
|
||
Adding `passive: true` informs the browser that this event will not be calling `preventDefault` and as such is safe to asynchronously dispatch, freeing up the main thread for lag-free operation. | ||
|
||
## See Also | ||
|
||
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Improving_scrolling_performance_with_passive_listeners |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# No Useless Passive | ||
|
||
This rule disallows setting `passive: true` for events on which it will have no effect. | ||
|
||
Events where `passive: true` has an effect are: `touchstart`, `touchmove`, `wheel`, and `mousewheel`. | ||
|
||
```js | ||
// bad (passive has no effect here) | ||
window.addEventListener('scroll', () => { /* ... */ }, { passive: true }) | ||
|
||
// good | ||
window.addEventListener('scroll', () => { /* ... */ }) | ||
``` | ||
|
||
## Why? | ||
|
||
Adding `passive: true` informs the browser that this event will not be calling `preventDefault` and as such is safe to asynchronously dispatch, freeing up the main thread for lag-free operation. However many events are not cancel-able and as such setting `passive: true` will have no effect on the browser. | ||
|
||
It is safe to leave the option set, but this may have a negative effect on code authors, as they might believe setting `passive: true` has a positive effect on their operations, leading to a false-confidence in code with `passive: true`. As such, removing the option where it has no effect demonstrates to the code author that this code will need to avoid expensive operations as this might have a detrimental affect on UI performance. | ||
|
||
## See Also | ||
|
||
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Improving_scrolling_performance_with_passive_listeners |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
# Prever Observers over Events | ||
|
||
Some events, such as `scroll` and `resize` have traditionally caused performance issues on web pages, as they are high frequency events, firing many times per second as the user interacts with the page viewport. | ||
|
||
## Scroll vs IntersectionObserver | ||
|
||
Typically `scroll` events are used to determine if an element is intersecting a viewport, which can result in expensive operations such as layout calculations, which has a detrimental affect on UI performance. Recent developments in web standards have introduced the `IntersectionObserver`, which is a more performant mechanism for determining if an element is intersecting the viewport. | ||
|
||
```js | ||
// bad, expensive, error-prone code to determine if the element is in the viewport; | ||
window.addEventListener('scroll', () => { | ||
const isIntersecting = checkIfElementIntersects(element.getBoundingClientRect(), window.innerHeight, document.clientHeight) | ||
element.classList.toggle('intersecting', isIntersecting) | ||
}) | ||
|
||
// good - more performant and less error-prone | ||
const observer = new IntersectionObserver(entries => { | ||
for(const {target, isIntersecting} of entries) { | ||
target.classList.toggle(target, isIntersecting) | ||
} | ||
}) | ||
observer.observe(element) | ||
``` | ||
|
||
## Resize vs ResizeObserver | ||
|
||
Similarly, `resize` events are typically used to determine if the viewport is smaller or larger than a certain size, similar to CSS media break points. Similar to the `IntersectionObserver` the `ResizeObserver` also exists as a more performant mechanism for observing changes to the viewport size. | ||
|
||
```js | ||
// bad, low-performing code to determine if the element is less than 500px large | ||
window.addEventListener('resize', () => { | ||
element.classList.toggle('size-small', element.getBoundingClientRect().width < 500) | ||
}) | ||
|
||
// good - more performant, only fires when the actual elements change size | ||
const observer = new ResizeObserver(entries => { | ||
for(const {target, contentRect} of entries) { | ||
target.classList.toggle('size-small', contentRect.width < 500) | ||
} | ||
}) | ||
observer.observe(element) | ||
``` | ||
|
||
## See Also | ||
|
||
https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API | ||
https://developer.mozilla.org/en-US/docs/Web/API/Resize_Observer_API |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,9 @@ module.exports = { | |
'github/no-blur': 'error', | ||
'github/no-dataset': 'error', | ||
'github/no-innerText': 'error', | ||
'github/unescaped-html-literal': 'error' | ||
'github/unescaped-html-literal': 'error', | ||
'github/no-useless-passive': 'error', | ||
'github/no-non-passive-high-frequency-events': 'error', | ||
'github/prefer-observers-over-events': 'error' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we consider this to be a breaking change? Do we want to release this as semver major? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To answer my own question, precedent exists for this to be semver minor, see #80 which was released as
keithamus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
const passiveEventListenerNames = new Set(['touchstart', 'touchmove', 'wheel', 'mousewheel']) | ||
|
||
const propIsPassiveTrue = prop => prop.key && prop.key.name === 'passive' && prop.value && prop.value.value === true | ||
|
||
module.exports = { | ||
meta: { | ||
docs: {} | ||
}, | ||
|
||
create(context) { | ||
return { | ||
['CallExpression[callee.property.name="addEventListener"]']: function(node) { | ||
const [name, listener, options] = node.arguments | ||
if (!listener) return | ||
if (name.type !== 'Literal') return | ||
if (!passiveEventListenerNames.has(name.value)) return | ||
if (options && options.type === 'ObjectExpression' && options.properties.some(propIsPassiveTrue)) return | ||
context.report(node, `High Frequency Events like "${name.value}" should be \`passive: true\``) | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
const passiveEventListenerNames = new Set(['touchstart', 'touchmove', 'wheel', 'mousewheel']) | ||
|
||
const propIsPassiveTrue = prop => prop.key && prop.key.name === 'passive' && prop.value && prop.value.value === true | ||
|
||
module.exports = { | ||
meta: { | ||
docs: {}, | ||
fixable: 'code' | ||
}, | ||
|
||
create(context) { | ||
return { | ||
['CallExpression[callee.property.name="addEventListener"]']: function(node) { | ||
const [name, listener, options] = node.arguments | ||
if (name.type !== 'Literal') return | ||
if (passiveEventListenerNames.has(name.value)) return | ||
if (options && options.type === 'ObjectExpression') { | ||
const i = options.properties.findIndex(propIsPassiveTrue) | ||
if (i === -1) return | ||
const passiveProp = options.properties[i] | ||
const l = options.properties.length | ||
const source = context.getSourceCode() | ||
context.report({ | ||
node: passiveProp, | ||
message: `"${name.value}" event listener is not cancellable and so \`passive: true\` does nothing.`, | ||
fix(fixer) { | ||
const removals = [] | ||
if (l === 1) { | ||
removals.push(options) | ||
removals.push(...source.getTokensBetween(listener, options)) | ||
} else { | ||
removals.push(passiveProp) | ||
if (i > 0) { | ||
removals.push(...source.getTokensBetween(options.properties[i - 1], passiveProp)) | ||
} else { | ||
removals.push(...source.getTokensBetween(passiveProp, options.properties[i + 1])) | ||
} | ||
} | ||
return removals.map(t => fixer.remove(t)) | ||
} | ||
}) | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
const observerMap = { | ||
scroll: 'IntersectionObserver', | ||
resize: 'ResizeObserver' | ||
} | ||
module.exports = { | ||
meta: { | ||
docs: {}, | ||
fixable: 'code' | ||
}, | ||
|
||
create(context) { | ||
return { | ||
['CallExpression[callee.property.name="addEventListener"]']: function(node) { | ||
const [name] = node.arguments | ||
if (name.type !== 'Literal') return | ||
if (!(name.value in observerMap)) return | ||
context.report({ | ||
node, | ||
message: `Avoid using "${name.value}" event listener. Consider using ${observerMap[name.value]} instead` | ||
}) | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
const rule = require('../lib/rules/no-non-passive-high-frequency-events') | ||
const RuleTester = require('eslint').RuleTester | ||
|
||
const ruleTester = new RuleTester() | ||
|
||
ruleTester.run('no-non-passive-high-frequency-events', rule, { | ||
valid: [ | ||
{ | ||
code: 'document.addEventListener("load", function(event) {})' | ||
}, | ||
{ | ||
code: 'document.addEventListener("click", function(event) {})' | ||
}, | ||
{ | ||
code: 'document.addEventListener("touchstart", function(event) {}, { passive: true })' | ||
}, | ||
{ | ||
code: 'el.addEventListener("touchstart", function(event) {}, { passive: true })' | ||
} | ||
], | ||
invalid: [ | ||
{ | ||
code: 'document.addEventListener("touchstart", function(event) {})', | ||
errors: [ | ||
{ | ||
message: 'High Frequency Events like "touchstart" should be `passive: true`', | ||
type: 'CallExpression' | ||
} | ||
] | ||
}, | ||
{ | ||
code: 'el.addEventListener("wheel", function(event) {})', | ||
errors: [ | ||
{ | ||
message: 'High Frequency Events like "wheel" should be `passive: true`', | ||
type: 'CallExpression' | ||
} | ||
] | ||
}, | ||
{ | ||
code: 'document.addEventListener("wheel", function(event) {})', | ||
errors: [ | ||
{ | ||
message: 'High Frequency Events like "wheel" should be `passive: true`', | ||
type: 'CallExpression' | ||
} | ||
] | ||
}, | ||
{ | ||
// Intentionally mispelled! | ||
code: 'document.addEventListener("wheel", function(event) {}, { pssive: true })', | ||
errors: [ | ||
{ | ||
message: 'High Frequency Events like "wheel" should be `passive: true`', | ||
type: 'CallExpression' | ||
} | ||
] | ||
}, | ||
{ | ||
code: 'document.addEventListener("wheel", function(event) {}, { passive: false })', | ||
errors: [ | ||
{ | ||
message: 'High Frequency Events like "wheel" should be `passive: true`', | ||
type: 'CallExpression' | ||
} | ||
] | ||
} | ||
] | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
const rule = require('../lib/rules/no-useless-passive') | ||
const RuleTester = require('eslint').RuleTester | ||
|
||
const ruleTester = new RuleTester() | ||
|
||
ruleTester.run('no-useless-passive', rule, { | ||
valid: [ | ||
{ | ||
code: 'document.addEventListener("scroll", function(event) {})' | ||
}, | ||
{ | ||
code: 'document.addEventListener("resize", function(event) {})' | ||
}, | ||
{ | ||
code: 'document.addEventListener("resize", function(event) {}, { passive: false })' | ||
} | ||
], | ||
invalid: [ | ||
{ | ||
code: 'document.addEventListener("scroll", function(event) {}, { passive: true })', | ||
output: 'document.addEventListener("scroll", function(event) {} )', | ||
errors: [ | ||
{ | ||
message: '"scroll" event listener is not cancellable and so `passive: true` does nothing.', | ||
type: 'Property' | ||
} | ||
] | ||
}, | ||
{ | ||
code: 'document.addEventListener("scroll", function(event) {}, { passive: true, foo: 1 })', | ||
output: 'document.addEventListener("scroll", function(event) {}, { foo: 1 })', | ||
errors: [ | ||
{ | ||
message: '"scroll" event listener is not cancellable and so `passive: true` does nothing.', | ||
type: 'Property' | ||
} | ||
] | ||
} | ||
] | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
const rule = require('../lib/rules/prefer-observers-over-events') | ||
const RuleTester = require('eslint').RuleTester | ||
|
||
const ruleTester = new RuleTester() | ||
|
||
ruleTester.run('prefer-observers-over-events', rule, { | ||
valid: [ | ||
{ | ||
code: 'document.addEventListener("touchstart", function(event) {})' | ||
} | ||
], | ||
invalid: [ | ||
{ | ||
code: 'document.addEventListener("scroll", function(event) {})', | ||
errors: [ | ||
{ | ||
message: 'Avoid using "scroll" event listener. Consider using IntersectionObserver instead', | ||
type: 'CallExpression' | ||
} | ||
] | ||
}, | ||
{ | ||
code: 'document.addEventListener("resize", function(event) {})', | ||
errors: [ | ||
{ | ||
message: 'Avoid using "resize" event listener. Consider using ResizeObserver instead', | ||
type: 'CallExpression' | ||
} | ||
] | ||
} | ||
] | ||
}) |
Uh oh!
There was an error while loading. Please reload this page.