From c24afd089812dce4b3a88b383eeb31546b1d078d Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Tue, 14 Jul 2020 16:43:36 +0100 Subject: [PATCH 1/7] feat: Add no-useless-passive rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kristján Oddsson --- docs/rules/no-useless-passive.md | 23 ++++++++++++++++ lib/rules/no-useless-passive.js | 46 ++++++++++++++++++++++++++++++++ tests/no-useless-passive.js | 40 +++++++++++++++++++++++++++ 3 files changed, 109 insertions(+) create mode 100644 docs/rules/no-useless-passive.md create mode 100644 lib/rules/no-useless-passive.js create mode 100644 tests/no-useless-passive.js diff --git a/docs/rules/no-useless-passive.md b/docs/rules/no-useless-passive.md new file mode 100644 index 00000000..1f8413f5 --- /dev/null +++ b/docs/rules/no-useless-passive.md @@ -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 diff --git a/lib/rules/no-useless-passive.js b/lib/rules/no-useless-passive.js new file mode 100644 index 00000000..f9eef57d --- /dev/null +++ b/lib/rules/no-useless-passive.js @@ -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)) + } + }) + } + } + } + } +} diff --git a/tests/no-useless-passive.js b/tests/no-useless-passive.js new file mode 100644 index 00000000..4ecabd3f --- /dev/null +++ b/tests/no-useless-passive.js @@ -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' + } + ] + } + ] +}) From 85d8b086725154e0e08a70d61e1aa54f130ccd60 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Tue, 14 Jul 2020 16:44:23 +0100 Subject: [PATCH 2/7] feat: add prefer-observers-over-events rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kristján Oddsson --- docs/rules/prefer-observers-over-events.md | 47 ++++++++++++++++++++++ lib/rules/prefer-observers-over-events.js | 24 +++++++++++ tests/prefer-observers-over-events.js | 32 +++++++++++++++ 3 files changed, 103 insertions(+) create mode 100644 docs/rules/prefer-observers-over-events.md create mode 100644 lib/rules/prefer-observers-over-events.js create mode 100644 tests/prefer-observers-over-events.js diff --git a/docs/rules/prefer-observers-over-events.md b/docs/rules/prefer-observers-over-events.md new file mode 100644 index 00000000..c9d24a37 --- /dev/null +++ b/docs/rules/prefer-observers-over-events.md @@ -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 diff --git a/lib/rules/prefer-observers-over-events.js b/lib/rules/prefer-observers-over-events.js new file mode 100644 index 00000000..0e9824a6 --- /dev/null +++ b/lib/rules/prefer-observers-over-events.js @@ -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` + }) + } + } + } +} diff --git a/tests/prefer-observers-over-events.js b/tests/prefer-observers-over-events.js new file mode 100644 index 00000000..9c7e0f47 --- /dev/null +++ b/tests/prefer-observers-over-events.js @@ -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' + } + ] + } + ] +}) From 6fd95708a42b858cb8ce4ca77c0b9207df9c06da Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Tue, 14 Jul 2020 16:45:07 +0100 Subject: [PATCH 3/7] feat: add no-non-passive-high-frequency-events rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kristján Oddsson --- .../no-non-passive-high-frequency-events.md | 21 ++++++ .../no-non-passive-high-frequency-events.js | 22 ++++++ tests/no-non-passive-high-frequency-events.js | 69 +++++++++++++++++++ 3 files changed, 112 insertions(+) create mode 100644 docs/rules/no-non-passive-high-frequency-events.md create mode 100644 lib/rules/no-non-passive-high-frequency-events.js create mode 100644 tests/no-non-passive-high-frequency-events.js diff --git a/docs/rules/no-non-passive-high-frequency-events.md b/docs/rules/no-non-passive-high-frequency-events.md new file mode 100644 index 00000000..4f642015 --- /dev/null +++ b/docs/rules/no-non-passive-high-frequency-events.md @@ -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 diff --git a/lib/rules/no-non-passive-high-frequency-events.js b/lib/rules/no-non-passive-high-frequency-events.js new file mode 100644 index 00000000..72b8a682 --- /dev/null +++ b/lib/rules/no-non-passive-high-frequency-events.js @@ -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\``) + } + } + } +} diff --git a/tests/no-non-passive-high-frequency-events.js b/tests/no-non-passive-high-frequency-events.js new file mode 100644 index 00000000..fd1399c6 --- /dev/null +++ b/tests/no-non-passive-high-frequency-events.js @@ -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' + } + ] + } + ] +}) From 1d3c50c01bc12e27b729275bce3382a8a21cfacd Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Tue, 14 Jul 2020 16:45:37 +0100 Subject: [PATCH 4/7] feat: add new eventlistener rules to configs/browser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kristján Oddsson --- lib/configs/browser.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/configs/browser.js b/lib/configs/browser.js index 3cf65f17..2930e656 100644 --- a/lib/configs/browser.js +++ b/lib/configs/browser.js @@ -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' } } From c2d9be4107162e653a98818c87afc8083188e5ff Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Thu, 16 Jul 2020 11:47:06 +0100 Subject: [PATCH 5/7] refactor: rename prever-observers-over-events --- .../{prefer-observers-over-events.md => prefer-observers.md} | 2 +- lib/configs/browser.js | 2 +- .../{prefer-observers-over-events.js => prefer-observers.js} | 0 tests/{prefer-observers-over-events.js => prefer-observers.js} | 0 4 files changed, 2 insertions(+), 2 deletions(-) rename docs/rules/{prefer-observers-over-events.md => prefer-observers.md} (98%) rename lib/rules/{prefer-observers-over-events.js => prefer-observers.js} (100%) rename tests/{prefer-observers-over-events.js => prefer-observers.js} (100%) diff --git a/docs/rules/prefer-observers-over-events.md b/docs/rules/prefer-observers.md similarity index 98% rename from docs/rules/prefer-observers-over-events.md rename to docs/rules/prefer-observers.md index c9d24a37..7597156a 100644 --- a/docs/rules/prefer-observers-over-events.md +++ b/docs/rules/prefer-observers.md @@ -1,4 +1,4 @@ -# Prever Observers over Events +# Prever Observers 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. diff --git a/lib/configs/browser.js b/lib/configs/browser.js index 2930e656..68358562 100644 --- a/lib/configs/browser.js +++ b/lib/configs/browser.js @@ -13,6 +13,6 @@ module.exports = { 'github/unescaped-html-literal': 'error', 'github/no-useless-passive': 'error', 'github/no-non-passive-high-frequency-events': 'error', - 'github/prefer-observers-over-events': 'error' + 'github/prefer-observers': 'error' } } diff --git a/lib/rules/prefer-observers-over-events.js b/lib/rules/prefer-observers.js similarity index 100% rename from lib/rules/prefer-observers-over-events.js rename to lib/rules/prefer-observers.js diff --git a/tests/prefer-observers-over-events.js b/tests/prefer-observers.js similarity index 100% rename from tests/prefer-observers-over-events.js rename to tests/prefer-observers.js From 357c8ecfa3e2806a6b3d31bef1f78e99c01ed221 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Thu, 16 Jul 2020 11:49:33 +0100 Subject: [PATCH 6/7] refactor: rename no-non-passive-high-frequency-events --- ...ive-high-frequency-events.md => require-passive-events.md} | 4 ++-- lib/configs/browser.js | 2 +- ...ive-high-frequency-events.js => require-passive-events.js} | 0 ...ive-high-frequency-events.js => require-passive-events.js} | 0 4 files changed, 3 insertions(+), 3 deletions(-) rename docs/rules/{no-non-passive-high-frequency-events.md => require-passive-events.md} (79%) rename lib/rules/{no-non-passive-high-frequency-events.js => require-passive-events.js} (100%) rename tests/{no-non-passive-high-frequency-events.js => require-passive-events.js} (100%) diff --git a/docs/rules/no-non-passive-high-frequency-events.md b/docs/rules/require-passive-events.md similarity index 79% rename from docs/rules/no-non-passive-high-frequency-events.md rename to docs/rules/require-passive-events.md index 4f642015..b860e8ef 100644 --- a/docs/rules/no-non-passive-high-frequency-events.md +++ b/docs/rules/require-passive-events.md @@ -1,6 +1,6 @@ -# No Non Passive High Frequency Events +# Require Passive Events -This rule disallows adding high frequency event listeners (`touchstart`, `touchmove`, `wheel`, `mousewheel`) without the `passive: true` option. +This rule enforces adding `passive: true` to high frequency event listeners (`touchstart`, `touchmove`, `wheel`, `mousewheel`). ```js // bad diff --git a/lib/configs/browser.js b/lib/configs/browser.js index 68358562..c2e5c446 100644 --- a/lib/configs/browser.js +++ b/lib/configs/browser.js @@ -12,7 +12,7 @@ module.exports = { 'github/no-innerText': 'error', 'github/unescaped-html-literal': 'error', 'github/no-useless-passive': 'error', - 'github/no-non-passive-high-frequency-events': 'error', + 'github/require-passive-events': 'error', 'github/prefer-observers': 'error' } } diff --git a/lib/rules/no-non-passive-high-frequency-events.js b/lib/rules/require-passive-events.js similarity index 100% rename from lib/rules/no-non-passive-high-frequency-events.js rename to lib/rules/require-passive-events.js diff --git a/tests/no-non-passive-high-frequency-events.js b/tests/require-passive-events.js similarity index 100% rename from tests/no-non-passive-high-frequency-events.js rename to tests/require-passive-events.js From 2c5cda0887b6d6825c2e4b3c6d13fb900bb745cf Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Thu, 16 Jul 2020 11:51:11 +0100 Subject: [PATCH 7/7] test: fix rule names --- tests/prefer-observers.js | 4 ++-- tests/require-passive-events.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/prefer-observers.js b/tests/prefer-observers.js index 9c7e0f47..8cdba73a 100644 --- a/tests/prefer-observers.js +++ b/tests/prefer-observers.js @@ -1,9 +1,9 @@ -const rule = require('../lib/rules/prefer-observers-over-events') +const rule = require('../lib/rules/prefer-observers') const RuleTester = require('eslint').RuleTester const ruleTester = new RuleTester() -ruleTester.run('prefer-observers-over-events', rule, { +ruleTester.run('prefer-observers', rule, { valid: [ { code: 'document.addEventListener("touchstart", function(event) {})' diff --git a/tests/require-passive-events.js b/tests/require-passive-events.js index fd1399c6..e5f3f22f 100644 --- a/tests/require-passive-events.js +++ b/tests/require-passive-events.js @@ -1,9 +1,9 @@ -const rule = require('../lib/rules/no-non-passive-high-frequency-events') +const rule = require('../lib/rules/require-passive-events') const RuleTester = require('eslint').RuleTester const ruleTester = new RuleTester() -ruleTester.run('no-non-passive-high-frequency-events', rule, { +ruleTester.run('require-passive-events', rule, { valid: [ { code: 'document.addEventListener("load", function(event) {})'