Skip to content
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

New rule: No unhandled scheduling #2

Merged
merged 5 commits into from
May 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
# eslint-plugin-enterprise-extras

This plugin adds extra ESLint rules that may be more suitable for an enterprise environment. The rules were created for use within Buildertrend, but feel free to request or propose any ESLint rules that may fall under this umbrella.

------------
# Installation

## Installation

Install this ESLint plugin as a dev dependency:

```bash
npm install --save-dev eslint-plugin-enterprise-extras
```
# Usage

## Usage

Edit your project's `.eslintrc.js` configuration to load the plugin:

```js
module.exports = {
plugins: ["enterprise-extras"],
Expand All @@ -19,7 +26,9 @@ module.exports = {
}
}
```

Alternatively, you could use the `recommended` or `all` preset rule configurations:

```js
module.exports = {
extends: ["plugin:enterprise-extras/recommended"],
Expand All @@ -29,12 +38,17 @@ module.exports = {
}
}
```
# Supported Rules

## Supported Rules

✅ = Recommended
🔧 = Auto-fixable
| Name | ✅ | 🔧 | Description |
| -------------------------------------------------- | - | - | ----------- |
| [no-href-assignment](/docs/no-href-assignment.md) | ✅ | 🔧 | Prefers `location.assign` instead of `location.href = ` |
| [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) |
# LICENSE
| [no-unhandled-scheduling](/docs/no-unhandled-scheduling.md) | ✅ | | `setTimeout` and `setInterval` calls should be cleared |

## LICENSE

MIT
8 changes: 7 additions & 1 deletion docs/no-href-assignment.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Disallow href direct assignment (`no-href-assignment`)

This rule prevents direct assignment to `window.location.href` (or similar) for redirection. Instead, `window.location.assign()` is preferred as is easier to mock when testing

## Rule Details

Examples of **incorrect** code for this rule:
Expand All @@ -23,7 +25,11 @@ myLoc.href.assign("http://example.com");
const location: { href: string } = { href: "" };
location.href = "test";
```

## When Not To Use It

When you are not concerned with mocking out href/url changes, or are indifferent on which method is used.

## Auto-fixable?
Yes ✔️

Yes ✔️
68 changes: 68 additions & 0 deletions docs/no-unhandled-scheduling.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Avoid unhandled scheduled tasks (`no-unhandled-scheduling`)

This rule suggests that all scheduled task calls (`setTimeout` or `setInterval`) should be handled in some way. In many cases, these scheduled tasks need to be cancelled to avoid leaking resources. React components, in particular, should take great care to ensure that these calls are cleared/released when the component gets unmounted.

Note that the rule **does not** ensure that the schedule handles are cleared at some point, but it does try to be sure that some explicit operation is at least performed on the handle, like an assignment to a variable to be cleared at a later point.

## Rule Details

Examples of **incorrect** code for this rule:

```typescript
setTimeout(() => {});

setInterval(() => {});

window.setInterval(() => {});

global.setTimeout(() => {});

class BadComponent extends React.Component {
componentDidMount() {
setTimeout(() => {
// Run operations like setState
})
}

render() {
return <></>;
}
}
```

Examples of **correct** code for this rule:

```typescript
const handle = setTimeout(() => {});
clearTimeout(handle);

// You can use the VOID keyword to bypass the check
void setInterval(() => {});

class GoodComponent extends React.Component {
timeoutHandle: null | NodeJS.Timeout = null;
componentDidMount() {
this.timeoutHandle = setTimeout(() => {
// Run operations like setState
})
}

componentWillUnmount() {
if(this.timeoutHandle) {
clearTimeout(this.timeoutHandle);
}
}

render() {
return <></>;
}
}
```

## When Not To Use It

If you do not care about cancelling scheduled tasks, or find your project uses scheduled tasks very infrequently.

## Auto-fixable?

No ❌
4 changes: 4 additions & 0 deletions docs/private-component-methods.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,11 @@ class GoodExample3 extends React.Component {
}
}
```

## When Not To Use It

If you are okay with public methods in your React components and find the pattern acceptable.

## Auto-fixable?

Yes ✔️
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@c-hess/eslint-plugin-enterprise-extras",
"description": "Extra eslint rules for enterprise environments focusing on React and Typescript",
"version": "1.0.1",
"version": "1.1.0",
"main": "dist/index.js",
"types": "dist/index.d.ts",
"files": [
Expand Down
6 changes: 5 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,28 @@
import noHrefAssignment from "./rules/no-href-assignment";
import noUnhandledScheduling from "./rules/no-unhandled-scheduling";
import privateComponentMethods from "./rules/private-component-methods";

export = {
rules: {
"no-href-assignment": noHrefAssignment,
"private-component-methods": privateComponentMethods
"private-component-methods": privateComponentMethods,
"no-unhandled-scheduling": noUnhandledScheduling,
},
configs: {
recommended: {
plugins: ["enterprise-extras"],
rules: {
"enterprise-extras/no-href-assignment": "error",
"enterprise-extras/private-component-methods": "error",
"enterprise-extras/no-unhandled-scheduling": "warn"
},
},
all: {
plugins: ["enterprise-extras"],
rules: {
"enterprise-extras/no-href-assignment": "error",
"enterprise-extras/private-component-methods": "error",
"enterprise-extras/no-unhandled-scheduling": "error"
},
}
}
Expand Down
10 changes: 5 additions & 5 deletions src/rules/no-href-assignment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const getParentObjectOfMemberExpression = (
};

const libDomFileNameRegex = /typescript.+lib.+lib\.dom\.d\.ts/
const isTypeDeclarationFromLibDom = (declaration: Declaration) => {
const isTypeDeclarationFromLibDom = (declaration: Declaration | undefined | null) => {
return libDomFileNameRegex.test(declaration?.getSourceFile().fileName);
};

Expand All @@ -38,20 +38,20 @@ const isWindowLocationType = (type: Type) => {

export default ESLintUtils.RuleCreator(
(name) =>
`https://github.com/C-Hess/eslint-plugin-cameron/blob/main/docs/${name}.md`
`https://github.com/C-Hess/eslint-plugin-enterprise-extras/blob/main/docs/${name}.md`
)<Options, MessageIds>({
name: "no-href-assignment",
meta: {
type: "suggestion",
fixable: "code",
docs: {
category: "Best Practices",
recommended: false,
description: "Prefer using location.assigned to make testing easier",
recommended: "error",
description: "Prefer using location.assign() to make testing easier",
},
messages: {
avoidHref:
"Prefer using location.assigned instead of href direct assignments",
"Prefer using location.assign() instead of href direct assignments",
},
schema: [],
},
Expand Down
70 changes: 70 additions & 0 deletions src/rules/no-unhandled-scheduling.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { ESLintUtils, TSESTree } from "@typescript-eslint/experimental-utils";
import { isAsExpression, isIdentifier } from "../utils/type-guards";

type MessageIds = "noUnhandledScheduling";
type Options = [];

const scheduleFuncRegex = /setTimeout|setInterval/;
const globalVarRegex = /window|global/;

export default ESLintUtils.RuleCreator(
(name) =>
`https://github.com/C-Hess/eslint-plugin-enterprise-extras/blob/main/docs/${name}.md`
)<Options, MessageIds>({
name: "no-unhandled-scheduling",
meta: {
type: "suggestion",
docs: {
category: "Best Practices",
recommended: "warn",
description:
"When using Javascript scheduling (`setTimeout` or `setInterval`), it is recommended to support cancelling of the task, especially within React components.",
},
messages: {
noUnhandledScheduling:
"Avoid scheduling uncancellable `{{ scheduleFunc }}` tasks. Use the returned handle to cancel the operation with `{{ scheduleClearFunc }}` when needed.",
},
schema: [],
},
defaultOptions: [],
create: function(context) {
const reportError = (callExpression: TSESTree.CallExpression) => {
let expression: TSESTree.Expression = callExpression.callee;

// Drill down expression if on global/window objects
if (expression.type === "MemberExpression") {
if (
isIdentifier(expression.object) &&
globalVarRegex.test(expression.object.name)
) {
expression = expression.property;
} else if (
isAsExpression(expression.object) &&
isIdentifier(expression.object.expression) &&
globalVarRegex.test(expression.object.expression.name)
) {
expression = expression.property;
}
}

// Once we have drilled down, test that we are at an identifier and that it is a schedule function
if (isIdentifier(expression) && scheduleFuncRegex.test(expression.name)) {
const funcName = expression.name;
const clearFuncName = funcName.replace("set", "clear");
context.report({
node: callExpression,
messageId: "noUnhandledScheduling",
data: {
scheduleFunc: funcName,
scheduleClearFunc: clearFuncName,
},
});
}
};

return {
"BlockStatement > ExpressionStatement > CallExpression": reportError,
"Program > ExpressionStatement > CallExpression": reportError,
};
},
});
4 changes: 2 additions & 2 deletions src/rules/private-component-methods.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,15 @@ const isLifecycleMethod = (

export default ESLintUtils.RuleCreator(
(name) =>
`https://github.com/C-Hess/eslint-plugin-cameron/blob/main/docs/${name}.md`
`https://github.com/C-Hess/eslint-plugin-enterprise-extras/blob/main/docs/${name}.md`
)<Options, MessageIds>({
name: "private-component-methods",
meta: {
type: "suggestion",
fixable: "code",
docs: {
category: "Best Practices",
recommended: false,
recommended: "error",
description:
"Non-lifecycle methods for React class components should be private to help find unused handlers",
},
Expand Down
9 changes: 9 additions & 0 deletions src/utils/type-guards.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,12 @@ export const isMethod = (
isMethodDefinition(node)
);
};

export const isIdentifier = (node: TSESTree.Expression): node is TSESTree.Identifier => {
return node.type === "Identifier";
}

export const isAsExpression = (node: TSESTree.Expression): node is TSESTree.TSAsExpression => {
return node.type === "TSAsExpression";
}

Loading