diff --git a/README.md b/README.md index 3e5461a..4c2ede4 100644 --- a/README.md +++ b/README.md @@ -48,6 +48,7 @@ module.exports = { | [no-href-assignment](/docs/no-href-assignment.md) | ✅ | 🔧 | Prefers `location.assign` instead of `location.href =` | | [private-component-methods](/docs/private-component-methods.md) | ✅ | 🔧 | Requires that all methods of react components are private (except reserved lifecycle methods) | | [no-unhandled-scheduling](/docs/no-unhandled-scheduling.md) | ✅ | | `setTimeout` and `setInterval` calls should be cleared | +| [unregister-events](/docs/unregister-events.md) | ✅ | | Ensures all events registered in React components are unregistered when component unmounts | ## LICENSE diff --git a/docs/unregister-events.md b/docs/unregister-events.md new file mode 100644 index 0000000..afe0ca3 --- /dev/null +++ b/docs/unregister-events.md @@ -0,0 +1,83 @@ +# Ensure that all events are unregistered when components are unmounted (`unregister-events`) + +This rule will check to make sure that all events that are unregistered have a corresponding unregister +call within `componentWillUnmount` or `useEffect` cleanup functions. + +Note: This component tries to make sure that the events are unregistered, but it is limited in it's detection and may require +the rule to be ignored during false-positives. + +## Rule Details + +Examples of **incorrect** code for this rule: + +```typescript +class BadClassComponent extends React.Component { + private onScroll = (e: any) => {} + + handleClick = () => { + window.addEventListener("scroll", this.onScroll) + }; + + render() { + return ; + } +} + +const BadHookComponent: React.FC = () => { + const onScroll = (e: any) => {} + + handleClick = () => { + window.addEventListener("scroll", onScroll) + }; + + render() { + return ; + } +} +``` + +Examples of **correct** code for this rule: + +```typescript +class GoodClassComponent extends React.Component { + private onScroll = (e: any) => {} + + componentWillUnmount() { + window.removeEventListener("scroll", this.onScroll) + } + + handleClick = () => { + window.addEventListener("scroll", this.onScroll) + }; + + render() { + return ; + } +} + +const GoodHookComponent: React.FC = () => { + const onScroll = (e: any) => {} + + handleClick = () => { + window.addEventListener("scroll", onScroll) + }; + + useEffect(() => { + return () => { + window.removeEventListener("scroll", onScroll); + } + }) + + render() { + return ; + } +} +``` + +## When Not To Use It + +If you find too many false-positives to make this rule worth it. + +## Auto-fixable? + +No ❌ diff --git a/package.json b/package.json index 7c0ef5d..5bca3ec 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "@buildertrend/eslint-plugin-enterprise-extras", "description": "Extra eslint rules for enterprise environments focusing on React and Typescript", - "version": "1.1.2", + "version": "2.0.0", "main": "dist/index.js", "types": "dist/index.d.ts", "files": [ diff --git a/src/index.ts b/src/index.ts index ea98acc..695cc30 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,12 +1,14 @@ import noHrefAssignment from "./rules/no-href-assignment"; import noUnhandledScheduling from "./rules/no-unhandled-scheduling"; import privateComponentMethods from "./rules/private-component-methods"; +import unregisterEvents from "./rules/unregister-events"; export = { rules: { "no-href-assignment": noHrefAssignment, "private-component-methods": privateComponentMethods, "no-unhandled-scheduling": noUnhandledScheduling, + "unregister-events": unregisterEvents, }, configs: { recommended: { @@ -15,6 +17,7 @@ export = { "enterprise-extras/no-href-assignment": "error", "enterprise-extras/private-component-methods": "error", "enterprise-extras/no-unhandled-scheduling": "warn", + "enterprise-extras/unregister-events": "error", }, }, all: { @@ -23,6 +26,7 @@ export = { "enterprise-extras/no-href-assignment": "error", "enterprise-extras/private-component-methods": "error", "enterprise-extras/no-unhandled-scheduling": "error", + "enterprise-extras/unregister-events": "error", }, }, }, diff --git a/src/rules/unregister-events.ts b/src/rules/unregister-events.ts new file mode 100644 index 0000000..e11a179 --- /dev/null +++ b/src/rules/unregister-events.ts @@ -0,0 +1,182 @@ +import { ESLintUtils, TSESTree } from "@typescript-eslint/experimental-utils"; + +type MessageIds = "unregisterEventsInClass" | "unregisterEventsInHook"; +type Options = []; + +interface ISubscriptionStack { + callExpression: TSESTree.Node; + eventHandler: TSESTree.Node; + eventName: string; +} + +export default ESLintUtils.RuleCreator( + (name) => + `https://github.com/buildertrend/eslint-plugin-enterprise-extras/blob/main/docs/${name}.md` +)({ + name: "unregister-events", + meta: { + type: "problem", + docs: { + category: "Possible Errors", + recommended: "error", + description: + "After registering event listeners in React components, event handlers should be unregistered when the component is unmounted.", + }, + messages: { + unregisterEventsInClass: + "`addEventListener` calls must have a corresponding unregister event call in `componentWillUnmount`", + unregisterEventsInHook: + "`addEventListener` calls must have a corresponding unregister event call in a `useEffect` cleanup function", + }, + schema: [], + }, + defaultOptions: [], + create: function (context) { + let stack: ISubscriptionStack[] = []; + + const isSameSubscription = ( + sub1: ISubscriptionStack, + sub2: ISubscriptionStack + ) => { + if (sub1.eventName === sub2.eventName) { + const handler1Tokens = context + .getSourceCode() + .getTokens(sub1.eventHandler); + const handler2Tokens = context + .getSourceCode() + .getTokens(sub2.eventHandler); + return ( + handler1Tokens.length === handler2Tokens.length && + handler1Tokens.every((t1, ind) => { + return t1.value === handler2Tokens[ind].value; + }) + ); + } + + return false; + }; + + const clearStack = () => { + stack = []; + }; + + const reportStack = (componentType: "hook" | "classComponent") => { + stack.forEach((error) => { + context.report({ + node: error.callExpression, + messageId: + componentType === "classComponent" + ? "unregisterEventsInClass" + : "unregisterEventsInHook", + }); + }); + + clearStack(); + }; + + const pushStack = (callExpression: TSESTree.CallExpression) => { + // If the callExpression fails these checks, chances are you have compiler errors anyways, so we can ignore adding to the stack + if ( + callExpression.arguments.length >= 2 && + callExpression.arguments.length <= 3 + ) { + let eventType = callExpression.arguments[0]; + let handler = callExpression.arguments[1]; + + if ( + eventType.type === "Literal" && + typeof eventType.value === "string" + ) { + const subscription = { + eventName: eventType.value, + eventHandler: handler, + callExpression: callExpression, + }; + stack.push(subscription); + } + } + }; + + const popStack = (callExpression: TSESTree.CallExpression) => { + // If the callExpression fails these checks, chances are you have compiler errors anyways, so we can ignore adding to the stack + if ( + callExpression.arguments.length >= 2 && + callExpression.arguments.length <= 3 + ) { + let eventType = callExpression.arguments[0]; + let handler = callExpression.arguments[1]; + + if ( + eventType.type === "Literal" && + typeof eventType.value === "string" + ) { + const eventName = eventType.value; + const subscription: ISubscriptionStack = { + callExpression: callExpression, + eventName: eventName, + eventHandler: handler, + }; + + stack = stack.filter((existingSubscription) => { + return !isSameSubscription(subscription, existingSubscription); + }); + } + } + }; + + return { + // Clear the stack between files to avoid memory leaks + Program: clearStack, + "Program:exit": clearStack, + + // Add event listener registrations made in class components to the stack + "ClassDeclaration[superClass.property.name=/Component|PureComponent/] CallExpression[callee.name='addEventListener']": + pushStack, + "ClassDeclaration[superClass.property.name=/Component|PureComponent/] CallExpression[callee.object.name=/window|document/][callee.property.name='addEventListener']": + pushStack, + + // Remove event listeners from the stack in class component componentWillUnmount methods + "ClassDeclaration[superClass.property.name=/Component|PureComponent/] MethodDefinition[key.name='componentWillUnmount'] CallExpression[callee.object.name=/window|document/][callee.property.name='removeEventListener']": + popStack, + "ClassDeclaration[superClass.property.name=/Component|PureComponent/] MethodDefinition[key.name='componentWillUnmount'] CallExpression[callee.name='removeEventListener']": + popStack, + "ClassDeclaration[superClass.property.name=/Component|PureComponent/] ClassProperty[key.name='componentWillUnmount'] CallExpression[callee.object.name=/window|document/][callee.property.name='removeEventListener']": + popStack, + "ClassDeclaration[superClass.property.name=/Component|PureComponent/] ClassProperty[key.name='componentWillUnmount'] CallExpression[callee.name='removeEventListener']": + popStack, + + // Add event listener registrations made in hook components to the stack + "VariableDeclarator[id.name=/^[A-Z].+/] CallExpression[callee.name='addEventListener']": + pushStack, + "VariableDeclarator[id.name=/^[A-Z].+/] CallExpression[callee.object.name=/window|document/][callee.property.name='addEventListener']": + pushStack, + "FunctionDeclaration[id.name=/^[A-Z].+/] CallExpression[callee.name='addEventListener']": + pushStack, + "FunctionDeclaration[id.name=/^[A-Z].+/] CallExpression[callee.object.name=/window|document/][callee.property.name='addEventListener']": + pushStack, + + // Remove event listeners from the stack in hook component useEffect cleanups + "VariableDeclarator[id.name=/^[A-Z].+/] CallExpression[callee.name='useEffect'] > ArrowFunctionExpression ReturnStatement CallExpression[callee.name='removeEventListener']": + popStack, + "VariableDeclarator[id.name=/^[A-Z].+/] CallExpression[callee.name='useEffect'] > ArrowFunctionExpression ReturnStatement CallExpression[callee.object.name=/window|document/][callee.property.name='removeEventListener']": + popStack, + "VariableDeclarator[id.name=/^[A-Z].+/] CallExpression[callee.object.name='React'][callee.property.name='useEffect'] > ArrowFunctionExpression ReturnStatement CallExpression[callee.name='removeEventListener']": + popStack, + "VariableDeclarator[id.name=/^[A-Z].+/] CallExpression[callee.object.name='React'][callee.property.name='useEffect'] > ArrowFunctionExpression ReturnStatement CallExpression[callee.object.name=/window|document/][callee.property.name='removeEventListener']": + popStack, + "FunctionDeclaration[id.name=/^[A-Z].+/] CallExpression[callee.name='useEffect'] > ArrowFunctionExpression ReturnStatement CallExpression[callee.name='removeEventListener']": + popStack, + "FunctionDeclaration[id.name=/^[A-Z].+/] CallExpression[callee.name='useEffect'] > ArrowFunctionExpression ReturnStatement CallExpression[callee.object.name=/window|document/][callee.property.name='removeEventListener']": + popStack, + "FunctionDeclaration[id.name=/^[A-Z].+/] CallExpression[callee.object.name='React'][callee.property.name='useEffect'] > ArrowFunctionExpression ReturnStatement CallExpression[callee.name='removeEventListener']": + popStack, + "FunctionDeclaration[id.name=/^[A-Z].+/] CallExpression[callee.object.name='React'][callee.property.name='useEffect'] > ArrowFunctionExpression ReturnStatement CallExpression[callee.object.name=/window|document/][callee.property.name='removeEventListener']": + popStack, + + // Report any event listeners not unregistered and still in the stack when leaving a class component/hook + "VariableDeclarator[id.name=/^[A-Z].+/]:exit": () => reportStack("hook"), + "FunctionDeclaration[id.name=/^[A-Z].+/]:exit": () => reportStack("hook"), + "ClassDeclaration:exit": () => reportStack("classComponent"), + }; + }, +}); diff --git a/tests/rules/unregister-events.test.ts b/tests/rules/unregister-events.test.ts new file mode 100644 index 0000000..2ff5272 --- /dev/null +++ b/tests/rules/unregister-events.test.ts @@ -0,0 +1,462 @@ +import rule from "../../src/rules/unregister-events"; +import { ESLintUtils } from "@typescript-eslint/experimental-utils"; +import { resolve, join } from "path"; + +const ruleTester = new ESLintUtils.RuleTester({ + parser: resolve("./node_modules/@typescript-eslint/parser") as any, + parserOptions: { + ecmaVersion: 2018, + tsconfigRootDir: join(__dirname, "../fixtures"), + project: "./tsconfig.json", + }, +}); + +ruleTester.run("unregister-events", rule, { + valid: [ + ` + window.addEventListener("scroll", () => {}); + `, + ` + addEventListener("scroll", () => {}); + `, + ` + class MyComponent extends React.Component { + private handler = () => { + + }; + + componentDidMount() { + window.addEventListener("scroll", handler); + } + + componentWillUnmount() { + window.removeEventListener("scroll", handler); + } + + render() { + return null; + } + } + `, + ` + class MyComponent extends React.Component { + private handler = () => { + + }; + + componentDidMount() { + window.addEventListener("scroll", handler); + } + + componentWillUnmount() { + removeEventListener("scroll", handler); + } + + render() { + return null; + } + } + `, + ` + class MyComponent extends React.Component { + private handler = () => { + + }; + + componentDidMount = () => { + window.addEventListener("scroll", handler); + } + + componentWillUnmount = () => { + window.removeEventListener("scroll", handler); + } + + render() { + return null; + } + } + `, + ` + class MyComponent extends React.PureComponent { + private handler = () => { + + }; + + componentDidMount() { + window.addEventListener("scroll", handler); + } + + componentWillUnmount() { + window.removeEventListener("scroll", handler); + } + + render() { + return null; + } + } + `, + ` + class MyComponent extends React.Component { + private handler = () => { + + }; + + componentDidMount() { + addEventListener("scroll", handler); + } + + componentWillUnmount() { + removeEventListener("scroll", handler); + } + + render() { + return null; + } + } + `, + ` + const MyComponent: React.FC = () => { + useEffect(() => { + const handler = () => {}; + + window.addEventListener("scroll", handler); + + return () => { + window.removeEventListener("scroll", handler); + } + }) + + return null; + } + `, + ` + const MyComponent: React.FC = () => { + useEffect(() => { + const handler = () => {}; + + addEventListener("scroll", handler); + + return () => { + removeEventListener("scroll", handler); + } + }) + + return null; + } + `, + ` + const MyComponent: React.FC = () => { + useEffect(() => { + const handler = () => {}; + + document.addEventListener("scroll", handler); + + return () => { + document.removeEventListener("scroll", handler); + } + }) + + return null; + } + `, + ` + function MyComponent() { + useEffect(() => { + const handler = () => {}; + + addEventListener("scroll", handler); + + return () => { + removeEventListener("scroll", handler); + } + }) + + return null; + } + `, + ` + const MyComponent: React.FC = () => { + React.useEffect(() => { + const handler = () => {}; + + addEventListener("scroll", handler); + + return () => { + removeEventListener("scroll", handler); + } + }) + + return null; + } + `, + ` + const MyComponent: React.FC = () => { + React.useEffect(() => { + const handler = () => {}; + + if (Math.random() > 0.5) { + addEventListener("scroll", handler); + + return () => { + removeEventListener("scroll", handler); + } + } + + return () => {}; + }) + + return null; + } + `, + ], + invalid: [ + { + code: ` + class MyComponent extends React.Component { + private handler = () => { + + }; + + componentDidMount() { + window.addEventListener("scroll", handler); + } + + render() { + return null; + } + } + `, + errors: [ + { + messageId: "unregisterEventsInClass", + }, + ], + }, + { + code: ` + class MyComponent extends React.Component { + private handler = () => { + + }; + + componentDidMount = () => { + window.addEventListener("scroll", handler); + } + + render() { + return null; + } + } + `, + errors: [ + { + messageId: "unregisterEventsInClass", + }, + ], + }, + { + code: ` + class MyComponent extends React.PureComponent { + private handler = () => { + + }; + + componentDidMount() { + window.addEventListener("scroll", handler); + } + + componentWillUnmount() { + // window.removeEventListener("scroll", handler); + } + + render() { + return null; + } + } + `, + errors: [ + { + messageId: "unregisterEventsInClass", + }, + ], + }, + { + code: ` + const MyComponent: React.FC = () => { + useEffect(() => { + const handler = () => {}; + + window.addEventListener("scroll", handler); + }) + + return null; + } + `, + errors: [ + { + messageId: "unregisterEventsInHook", + }, + ], + }, + { + code: ` + const MyComponent: React.FC = () => { + useEffect(() => { + const handler = () => {}; + + addEventListener("scroll", handler); + }) + + return null; + } + `, + errors: [ + { + messageId: "unregisterEventsInHook", + }, + ], + }, + { + code: ` + function MyComponent() { + useEffect(() => { + const handler = () => {}; + + addEventListener("scroll", handler); + }) + + return null; + } + `, + errors: [ + { + messageId: "unregisterEventsInHook", + }, + ], + }, + { + code: ` + const MyComponent: React.FC = () => { + React.useEffect(() => { + const handler = () => {}; + + addEventListener("scroll", handler); + }) + + return null; + } + `, + errors: [ + { + messageId: "unregisterEventsInHook", + }, + ], + }, + { + code: ` + const MyComponent: React.FC = () => { + React.useEffect(() => { + const handler = () => {}; + + if (Math.random() > 0.5) { + addEventListener("scroll", handler); + } + + return () => {}; + }) + + return null; + } + `, + errors: [ + { + messageId: "unregisterEventsInHook", + }, + ], + }, + { + code: ` + const MyComponent: React.FC = () => { + useEffect(() => { + const handler = () => {}; + + addEventListener("scroll", handler); + + () => { + removeEventListener("scroll", handler); + }; + }) + + return null; + } + `, + errors: [ + { + messageId: "unregisterEventsInHook", + }, + ], + }, + { + code: ` + const MyComponent: React.FC = () => { + useEffect(() => { + const handler = () => {}; + + addEventListener("scroll", handler); + + () => { + removeEventListener("notScroll", handler); + }; + }) + + return null; + } + `, + errors: [ + { + messageId: "unregisterEventsInHook", + }, + ], + }, + { + code: ` + const MyComponent: React.FC = () => { + useEffect(() => { + const handler = () => {}; + + addEventListener("scroll1", handler); + addEventListener("scroll2", handler); + }) + + return null; + } + `, + errors: [ + { + messageId: "unregisterEventsInHook", + }, + { + messageId: "unregisterEventsInHook", + }, + ], + }, + { + code: ` + const MyComponent: React.FC = () => { + useEffect(() => { + const handler = () => {}; + + document.addEventListener("scroll", handler); + }) + + return null; + } + `, + errors: [ + { + messageId: "unregisterEventsInHook", + }, + ], + }, + ], +});