Add event clearing to prefer-add-event-listener rule (#216)

sindresorhus · Jan 30, 2019 · 7503d12 · 7503d12
1 parent e403caf
commit 7503d12
Show file tree

Hide file tree

Showing 3 changed files with 73 additions and 17 deletions.
diff --git a/docs/rules/prefer-add-event-listener.md b/docs/rules/prefer-add-event-listener.md
@@ -1,8 +1,8 @@
-# Prefer `addEventListener` over `on`-functions
+# Prefer `addEventListener` and `removeEventListener` over `on`-functions
 
-Enforces the use of, for example, `foo.addEventListener('click', handler);` over `foo.onclick = handler;` for HTML DOM Events. There are [numerous advantages of using `addEventListener`](https://stackoverflow.com/questions/6348494/addeventlistener-vs-onclick/35093997#35093997). Some of these advantages include registering unlimited event handlers and optionally having the event handler invoked only once.
+Enforces the use of `addEventListener` and `removeEventListener` over their `on` counterparts. For example, `foo.addEventListener('click', handler);` is preferred over `foo.onclick = handler;` for HTML DOM Events. There are [numerous advantages of using `addEventListener`](https://stackoverflow.com/questions/6348494/addeventlistener-vs-onclick/35093997#35093997). Some of these advantages include registering unlimited event handlers and optionally having the event handler invoked only once.
 
-This rule is fixable.
+This rule is fixable (only for `addEventListener`).
 
 
 ## Fail
@@ -19,6 +19,10 @@ foo.onkeydown = () => {};
 foo.bar.onclick = onClick;
 ```
 
+```js
+foo.onclick = null;
+```
+
 ## Pass
 
 ```js
@@ -32,3 +36,7 @@ foo.addEventListener('keydown', () => {});
 ```js
 foo.bar.addEventListener('click', onClick);
 ```
+
+```js
+foo.removeEventListener('click', onClick);
+```
diff --git a/rules/prefer-add-event-listener.js b/rules/prefer-add-event-listener.js
@@ -9,8 +9,8 @@ const getEventTypeName = eventMethodName => eventMethodName.slice('on'.length);
 
 const beforeUnloadMessage = 'Use `event.preventDefault(); event.returnValue = \'foo\'` to trigger the prompt.';
 
-const formatMessage = (eventMethodName, extra) => {
-	let message = `Prefer \`addEventListener\` over \`${eventMethodName}\`.`;
+const formatMessage = (methodReplacement, eventMethodName, extra) => {
+	let message = `Prefer \`${methodReplacement}\` over \`${eventMethodName}\`.`;
 
 	if (extra) {
 		message += ' ' + extra;
@@ -42,6 +42,18 @@ const shouldFixBeforeUnload = (assignedExpression, nodeReturnsSomething) => {
 	return !nodeReturnsSomething.get(assignedExpression);
 };
 
+const isClearing = node => {
+	if (node.type === 'Literal') {
+		return node.raw === 'null';
+	}
+
+	if (node.type === 'Identifier') {
+		return node.name === 'undefined';
+	}
+
+	return false;
+};
+
 const create = context => {
 	const nodeReturnsSomething = new WeakMap();
 	let codePathInfo = null;
@@ -83,22 +95,25 @@ const create = context => {
 				return;
 			}
 
-			if (eventTypeName === 'beforeunload' &&
+			if (isClearing(assignedExpression)) {
+				context.report({
+					node,
+					message: formatMessage('removeEventListener', eventMethodName)
+				});
+			} else if (eventTypeName === 'beforeunload' &&
 				!shouldFixBeforeUnload(assignedExpression, nodeReturnsSomething)
 			) {
 				context.report({
 					node,
-					message: formatMessage(eventMethodName, beforeUnloadMessage)
+					message: formatMessage('addEventListener', eventMethodName, beforeUnloadMessage)
+				});
+			} else {
+				context.report({
+					node,
+					message: formatMessage('addEventListener', eventMethodName),
+					fix: fixer => fix(fixer, context.getSourceCode(), node, memberExpression)
 				});
-
-				return;
 			}
-
-			context.report({
-				node,
-				message: formatMessage(eventMethodName),
-				fix: fixer => fix(fixer, context.getSourceCode(), node, memberExpression)
-			});
 		}
 	};
 };

diff --git a/test/prefer-add-event-listener.js b/test/prefer-add-event-listener.js
@@ -26,6 +26,7 @@ const expectedBeforeUnloadWithReturnMessage = [
 ruleTester.run('prefer-add-event-listener', rule, {
 	valid: [
 		'foo.addEventListener(\'click\', () => {})',
+		'foo.removeEventListener(\'click\', onClick)',
 		'foo.onclick',
 		'foo.setCallBack = () => {console.log(\'foo\')}',
 		'setCallBack = () => {console.log(\'foo\')}',
@@ -39,11 +40,21 @@ ruleTester.run('prefer-add-event-listener', rule, {
 			'foo.addEventListener(\'click\', () => {})',
 			'onclick'
 		),
+		invalidTestCase(
+			'foo.onclick = 1',
+			'foo.addEventListener(\'click\', 1)',
+			'onclick'
+		),
 		invalidTestCase(
 			'foo.bar.onclick = onClick',
 			'foo.bar.addEventListener(\'click\', onClick)',
 			'onclick'
 		),
+		invalidTestCase(
+			'const bar = null; foo.onclick = bar;',
+			'const bar = null; foo.addEventListener(\'click\', bar);',
+			'onclick'
+		),
 		invalidTestCase(
 			'foo.onkeydown = () => {}',
 			'foo.addEventListener(\'keydown\', () => {})',
@@ -63,7 +74,30 @@ ruleTester.run('prefer-add-event-listener', rule, {
 			})`,
 			'onclick'
 		),
-
+		invalidTestCase(
+			'foo.onclick = null',
+			null,
+			null,
+			'Prefer `removeEventListener` over `onclick`.'
+		),
+		invalidTestCase(
+			'foo.onclick = undefined',
+			null,
+			null,
+			'Prefer `removeEventListener` over `onclick`.'
+		),
+		invalidTestCase(
+			'window.onbeforeunload = null',
+			null,
+			null,
+			'Prefer `removeEventListener` over `onbeforeunload`.'
+		),
+		invalidTestCase(
+			'window.onbeforeunload = undefined',
+			null,
+			null,
+			'Prefer `removeEventListener` over `onbeforeunload`.'
+		),
 		invalidTestCase(
 			'window.onbeforeunload = foo',
 			null,
@@ -92,7 +126,6 @@ ruleTester.run('prefer-add-event-listener', rule, {
 			null,
 			expectedBeforeUnloadWithReturnMessage
 		),
-
 		invalidTestCase(
 			`window.onbeforeunload = function () {
 				return;