From dcab30154479d7ae306b8313b06e6b03558d83a1 Mon Sep 17 00:00:00 2001 From: Lucy Lin Date: Fri, 16 Dec 2022 18:59:20 -0800 Subject: [PATCH 1/4] Add new rule --- README.md | 1 + docs/rules/no-runloop.md | 71 ++++++++++++++++ lib/rules/no-runloop.js | 135 +++++++++++++++++++++++++++++++ tests/lib/rules/no-runloop.js | 148 ++++++++++++++++++++++++++++++++++ 4 files changed, 355 insertions(+) create mode 100644 docs/rules/no-runloop.md create mode 100644 lib/rules/no-runloop.js create mode 100644 tests/lib/rules/no-runloop.js diff --git a/README.md b/README.md index b5aa064bc8..6fd59e211d 100644 --- a/README.md +++ b/README.md @@ -155,6 +155,7 @@ module.exports = { | [no-incorrect-calls-with-inline-anonymous-functions](docs/rules/no-incorrect-calls-with-inline-anonymous-functions.md) | disallow inline anonymous functions as arguments to `debounce`, `once`, and `scheduleOnce` | ✅ | | | | [no-invalid-debug-function-arguments](docs/rules/no-invalid-debug-function-arguments.md) | disallow usages of Ember's `assert()` / `warn()` / `deprecate()` functions that have the arguments passed in the wrong order. | ✅ | | | | [no-restricted-property-modifications](docs/rules/no-restricted-property-modifications.md) | disallow modifying the specified properties | | 🔧 | | +| [no-runloop](docs/rules/no-runloop.md) | disallow usage of `@ember/runloop` functions | | | | | [require-fetch-import](docs/rules/require-fetch-import.md) | enforce explicit import for `fetch()` | | | | ### Routes diff --git a/docs/rules/no-runloop.md b/docs/rules/no-runloop.md new file mode 100644 index 0000000000..bad0bb8948 --- /dev/null +++ b/docs/rules/no-runloop.md @@ -0,0 +1,71 @@ +# ember/no-runloop + + + +Ember's runloop functions are not lifecycle-aware and don't ensure that an object's async is cleaned up. It is recommended to use [`ember-lifeline`](https://ember-lifeline.github.io/ember-lifeline/), [`ember-concurrency`](http://ember-concurrency.com/docs/introduction/), or [`@ember/destroyable`](https://rfcs.emberjs.com/id/0580-destroyables/) instead. + +## Rule Details + +This rule disallows usage of `@ember/runloop` functions. + +## Examples + +Example of **incorrect** code for this rule: + +```ts +import Component from '@glimmer/component'; +import { run } from '@ember/runloop'; + +export default class MyComponent extends Component { + constructor() { + super(...arguments); + + run.later(() => { + this.set('date', new Date()); + }, 500); + } +} +``` + +Example of **correct** code for this rule using `ember-lifeline`: + +```js +import Component from '@glimmer/component'; +import { runTask, runDisposables } from 'ember-lifeline'; + +export default class MyComponent extends Component { + constructor(...args) { + super(...args); + + runTask( + this, + () => { + this.set('date', new Date()); + }, + 500 + ); + } + + willDestroy(...args) { + super.willDestroy(...args); + + runDisposables(this); + } +} +``` + +## Configuration + +If you have `@ember/runloop` functions that you wish to allow, you can configure this rule to allow specific methods. The configuration takes an array of strings, where the strings must be names of runloop functions. + +```ts +module.exports = { + rules: { + 'ember/no-runloop': ['error', ['debounce', 'begin', 'end']], + }, +}; +``` + +## References + +- [require-lifeline](https://github.com/ember-best-practices/eslint-plugin-ember-best-practices/blob/master/guides/rules/require-ember-lifeline.md) - a rule that was originally implemented in eslint-plugin-ember-best-practices diff --git a/lib/rules/no-runloop.js b/lib/rules/no-runloop.js new file mode 100644 index 0000000000..2924e2e2f8 --- /dev/null +++ b/lib/rules/no-runloop.js @@ -0,0 +1,135 @@ +'use strict'; + +const { getImportIdentifier } = require('../utils/import'); +const { isIdentifier, isMemberExpression } = require('../utils/types'); + +//------------------------------------------------------------------------------ +// General rule - Don’t use runloop functions +//------------------------------------------------------------------------------ + +/** + * Map of runloop functions to ember-lifeline recommended replacements + */ +const RUNLOOP_TO_LIFELINE_MAP = Object.freeze({ + later: 'runTask', + next: 'runTask', + debounce: 'debounceTask', + schedule: 'scheduleTask', + throttle: 'throttleTask', +}); + +const ERROR_MESSAGE = + "Don't use @ember/runloop functions. Use ember-lifeline, ember-concurrency, or @ember/destroyable instead."; + +// https://api.emberjs.com/ember/3.24/classes/@ember%2Frunloop +const EMBER_RUNLOOP_FUNCTIONS = [ + 'begin', + 'bind', + 'cancel', + 'debounce', + 'end', + 'join', + 'later', + 'next', + 'once', + 'run', + 'schedule', + 'scheduleOnce', + 'throttle', +]; + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + type: 'problem', + docs: { + description: 'disallow usage of `@ember/runloop` functions', + category: 'Miscellaneous', + url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-runloop.md', + }, + fixable: null, + schema: [ + { + type: 'array', + uniqueItems: true, + items: { type: 'string', enum: EMBER_RUNLOOP_FUNCTIONS }, + }, + ], + messages: { + main: ERROR_MESSAGE, + lifelineReplacement: `${ERROR_MESSAGE} For this case, you can replace \`{{actualMethodUsed}}\` with \`{{lifelineEquivalent}}\` from ember-lifeline.`, + }, + }, + + create(context) { + // List of allowed runloop functions + const allowList = context.options[0] ?? []; + // Maps local names to imported names + const localMap = {}; + + /** + * Reports a node with usage of a disallowed runloop function + * @param {Node} node + * @param {String} [runloopFn] the name of the runloop function that is not allowed + * @param {String} [localName] the locally used name of the runloop function + */ + const report = function (node, runloopFn, localName) { + if (Object.keys(RUNLOOP_TO_LIFELINE_MAP).includes(runloopFn)) { + // If there is a recommended lifeline replacement, include the suggestion + context.report({ + node, + messageId: 'lifelineReplacement', + data: { + actualMethodUsed: localName, + lifelineEquivalent: RUNLOOP_TO_LIFELINE_MAP[runloopFn], + }, + }); + } else { + // Otherwise, show a generic error message + context.report({ node, messageId: 'main' }); + } + }; + + return { + ImportDeclaration(node) { + if (node.source.value === '@ember/runloop') { + for (const fn of EMBER_RUNLOOP_FUNCTIONS) { + const importIdentifier = getImportIdentifier(node, '@ember/runloop', fn); + localMap[importIdentifier] = fn; + } + } + }, + + CallExpression(node) { + // Examples: run(...), later(...) + if (isIdentifier(node.callee)) { + const name = node.callee.name; + const runloopFn = localMap[name]; + const isNotAllowed = runloopFn && !allowList.includes(runloopFn); + if (isNotAllowed) { + report(node, runloopFn, name); + } + } + + // runloop functions (aside from run itself) can chain onto `run`, so we need to check for this + // Examples: run.later(...), run.schedule(...) + if (isMemberExpression(node.callee) && isIdentifier(node.callee.object)) { + const objectName = node.callee.object.name; + const objectRunloopFn = localMap[objectName]; + + if (objectRunloopFn === 'run' && isIdentifier(node.callee.property)) { + const runloopFn = node.callee.property.name; + + if ( + EMBER_RUNLOOP_FUNCTIONS.includes(runloopFn) && + runloopFn !== 'run' && + !allowList.includes(runloopFn) + ) { + report(node, runloopFn, `${objectName}.${runloopFn}`); + } + } + } + }, + }; + }, +}; diff --git a/tests/lib/rules/no-runloop.js b/tests/lib/rules/no-runloop.js new file mode 100644 index 0000000000..7d228d8782 --- /dev/null +++ b/tests/lib/rules/no-runloop.js @@ -0,0 +1,148 @@ +// ------------------------------------------------------------------------------ +// Requirements +// ------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/no-runloop'); +const RuleTester = require('eslint').RuleTester; + +// ------------------------------------------------------------------------------ +// Tests +// ------------------------------------------------------------------------------ + +const eslintTester = new RuleTester({ + parserOptions: { ecmaVersion: 2020, sourceType: 'module' }, +}); + +eslintTester.run('no-runloop', rule, { + valid: [ + ` + run(); + later(); + `, + ` + import { run } from 'foobar'; + run(); + `, + ` + import { run } from '@ember/runloop'; + this.run(); + runTask(); + runRun(); + `, + { + code: ` + import { run } from '@ember/runloop'; + run(); + later(); + `, + options: [['run']], + }, + { + code: ` + import { run as foobar } from '@ember/runloop'; + foobar(); + `, + options: [['run']], + }, + ` + import run from '@ember/runloop'; + run(); + `, + ` + import { run } from '@ember/runloop'; + run.run(); + run.foobar(); + `, + ` + import { later } from '@ember/runloop'; + later.run(); + later.foobar(); + `, + { + code: ` + import { run } from '@ember/runloop'; + run.later(); + `, + options: [['later']], + }, + ], + invalid: [ + { + code: ` + import { run } from '@ember/runloop'; + run(); + `, + output: null, + errors: [{ messageId: 'main' }], + }, + { + code: ` + import { run as foobar } from '@ember/runloop'; + foobar(); + `, + output: null, + errors: [{ messageId: 'main' }], + }, + { + code: ` + import { later } from '@ember/runloop'; + later(); + `, + output: null, + errors: [{ messageId: 'lifelineReplacement' }], + }, + { + code: ` + import { later as foobar } from '@ember/runloop'; + foobar(); + `, + output: null, + errors: [{ messageId: 'lifelineReplacement' }], + }, + { + code: ` + import { run, later } from '@ember/runloop'; + run(); + later(); + `, + output: null, + options: [['run']], + errors: [{ messageId: 'lifelineReplacement' }], + }, + { + code: ` + import { run, later } from '@ember/runloop'; + run(); + later(); + `, + output: null, + options: [['later']], + errors: [{ messageId: 'main' }], + }, + // chaining off of `run` + { + code: ` + import { run } from '@ember/runloop'; + run.later(); + `, + output: null, + errors: [{ messageId: 'lifelineReplacement' }], + }, + { + code: ` + import { run } from '@ember/runloop'; + run.begin(); + `, + output: null, + errors: [{ messageId: 'main' }], + }, + { + code: ` + import { run as foobar } from '@ember/runloop'; + foobar.schedule(); + `, + output: null, + errors: [{ messageId: 'lifelineReplacement' }], + }, + ], +}); From af4498b785deadb2ee2aa2412250fc8219ca1fc2 Mon Sep 17 00:00:00 2001 From: Lucy Lin Date: Sun, 18 Dec 2022 22:34:39 -0800 Subject: [PATCH 2/4] Address comments --- docs/rules/no-runloop.md | 9 ++++- lib/rules/no-runloop.js | 45 +++++++++++++-------- tests/lib/rules/no-runloop.js | 73 ++++++++++++++++++++++++++++------- 3 files changed, 94 insertions(+), 33 deletions(-) diff --git a/docs/rules/no-runloop.md b/docs/rules/no-runloop.md index bad0bb8948..f1232fec7d 100644 --- a/docs/rules/no-runloop.md +++ b/docs/rules/no-runloop.md @@ -56,12 +56,17 @@ export default class MyComponent extends Component { ## Configuration -If you have `@ember/runloop` functions that you wish to allow, you can configure this rule to allow specific methods. The configuration takes an array of strings, where the strings must be names of runloop functions. +If you have `@ember/runloop` functions that you wish to allow, you can configure this rule to allow specific methods. The configuration takes an object with the `allowList` property, which is an array of strings where the strings must be names of runloop functions. ```ts module.exports = { rules: { - 'ember/no-runloop': ['error', ['debounce', 'begin', 'end']], + 'ember/no-runloop': [ + 'error', + { + allowList: ['debounce', 'begin', 'end'], + }, + ], }, }; ``` diff --git a/lib/rules/no-runloop.js b/lib/rules/no-runloop.js index 2924e2e2f8..0ea3d45b6a 100644 --- a/lib/rules/no-runloop.js +++ b/lib/rules/no-runloop.js @@ -1,8 +1,5 @@ 'use strict'; -const { getImportIdentifier } = require('../utils/import'); -const { isIdentifier, isMemberExpression } = require('../utils/types'); - //------------------------------------------------------------------------------ // General rule - Don’t use runloop functions //------------------------------------------------------------------------------ @@ -50,9 +47,19 @@ module.exports = { fixable: null, schema: [ { - type: 'array', - uniqueItems: true, - items: { type: 'string', enum: EMBER_RUNLOOP_FUNCTIONS }, + type: 'object', + properties: { + allowList: { + type: 'array', + uniqueItems: true, + items: { + type: 'string', + enum: EMBER_RUNLOOP_FUNCTIONS, + minItems: 1, + }, + }, + }, + additionalProperties: false, }, ], messages: { @@ -63,9 +70,9 @@ module.exports = { create(context) { // List of allowed runloop functions - const allowList = context.options[0] ?? []; - // Maps local names to imported names - const localMap = {}; + const allowList = context.options[0]?.allowList ?? []; + // Maps local names to imported names of imports + const localToImportedNameMap = {}; /** * Reports a node with usage of a disallowed runloop function @@ -93,18 +100,22 @@ module.exports = { return { ImportDeclaration(node) { if (node.source.value === '@ember/runloop') { - for (const fn of EMBER_RUNLOOP_FUNCTIONS) { - const importIdentifier = getImportIdentifier(node, '@ember/runloop', fn); - localMap[importIdentifier] = fn; + for (const spec of node.specifiers) { + if (spec.type === 'ImportSpecifier') { + const importedName = spec.imported.name; + if (EMBER_RUNLOOP_FUNCTIONS.includes(importedName)) { + localToImportedNameMap[spec.local.name] = importedName; + } + } } } }, CallExpression(node) { // Examples: run(...), later(...) - if (isIdentifier(node.callee)) { + if (node.callee.type === 'Identifier') { const name = node.callee.name; - const runloopFn = localMap[name]; + const runloopFn = localToImportedNameMap[name]; const isNotAllowed = runloopFn && !allowList.includes(runloopFn); if (isNotAllowed) { report(node, runloopFn, name); @@ -113,11 +124,11 @@ module.exports = { // runloop functions (aside from run itself) can chain onto `run`, so we need to check for this // Examples: run.later(...), run.schedule(...) - if (isMemberExpression(node.callee) && isIdentifier(node.callee.object)) { + if (node.callee.type === 'MemberExpression' && node.callee.object?.type === 'Identifier') { const objectName = node.callee.object.name; - const objectRunloopFn = localMap[objectName]; + const objectRunloopFn = localToImportedNameMap[objectName]; - if (objectRunloopFn === 'run' && isIdentifier(node.callee.property)) { + if (objectRunloopFn === 'run' && node.callee.property?.type === 'Identifier') { const runloopFn = node.callee.property.name; if ( diff --git a/tests/lib/rules/no-runloop.js b/tests/lib/rules/no-runloop.js index 7d228d8782..953a420822 100644 --- a/tests/lib/rules/no-runloop.js +++ b/tests/lib/rules/no-runloop.js @@ -35,14 +35,14 @@ eslintTester.run('no-runloop', rule, { run(); later(); `, - options: [['run']], + options: [{ allowList: ['run'] }], }, { code: ` import { run as foobar } from '@ember/runloop'; foobar(); `, - options: [['run']], + options: [{ allowList: ['run'] }], }, ` import run from '@ember/runloop'; @@ -63,7 +63,7 @@ eslintTester.run('no-runloop', rule, { import { run } from '@ember/runloop'; run.later(); `, - options: [['later']], + options: [{ allowList: ['later'] }], }, ], invalid: [ @@ -73,7 +73,12 @@ eslintTester.run('no-runloop', rule, { run(); `, output: null, - errors: [{ messageId: 'main' }], + errors: [ + { + messageId: 'main', + type: 'CallExpression', + }, + ], }, { code: ` @@ -81,7 +86,12 @@ eslintTester.run('no-runloop', rule, { foobar(); `, output: null, - errors: [{ messageId: 'main' }], + errors: [ + { + messageId: 'main', + type: 'CallExpression', + }, + ], }, { code: ` @@ -89,7 +99,12 @@ eslintTester.run('no-runloop', rule, { later(); `, output: null, - errors: [{ messageId: 'lifelineReplacement' }], + errors: [ + { + messageId: 'lifelineReplacement', + type: 'CallExpression', + }, + ], }, { code: ` @@ -97,7 +112,12 @@ eslintTester.run('no-runloop', rule, { foobar(); `, output: null, - errors: [{ messageId: 'lifelineReplacement' }], + errors: [ + { + messageId: 'lifelineReplacement', + type: 'CallExpression', + }, + ], }, { code: ` @@ -106,8 +126,13 @@ eslintTester.run('no-runloop', rule, { later(); `, output: null, - options: [['run']], - errors: [{ messageId: 'lifelineReplacement' }], + options: [{ allowList: ['run'] }], + errors: [ + { + messageId: 'lifelineReplacement', + type: 'CallExpression', + }, + ], }, { code: ` @@ -116,8 +141,13 @@ eslintTester.run('no-runloop', rule, { later(); `, output: null, - options: [['later']], - errors: [{ messageId: 'main' }], + options: [{ allowList: ['later'] }], + errors: [ + { + messageId: 'main', + type: 'CallExpression', + }, + ], }, // chaining off of `run` { @@ -126,7 +156,12 @@ eslintTester.run('no-runloop', rule, { run.later(); `, output: null, - errors: [{ messageId: 'lifelineReplacement' }], + errors: [ + { + messageId: 'lifelineReplacement', + type: 'CallExpression', + }, + ], }, { code: ` @@ -134,7 +169,12 @@ eslintTester.run('no-runloop', rule, { run.begin(); `, output: null, - errors: [{ messageId: 'main' }], + errors: [ + { + messageId: 'main', + type: 'CallExpression', + }, + ], }, { code: ` @@ -142,7 +182,12 @@ eslintTester.run('no-runloop', rule, { foobar.schedule(); `, output: null, - errors: [{ messageId: 'lifelineReplacement' }], + errors: [ + { + messageId: 'lifelineReplacement', + type: 'CallExpression', + }, + ], }, ], }); From b2c0562f3999baf29255ca10c401ff7b2da9a39f Mon Sep 17 00:00:00 2001 From: Lucy Lin Date: Mon, 19 Dec 2022 14:03:42 -0800 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com> --- docs/rules/no-runloop.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/rules/no-runloop.md b/docs/rules/no-runloop.md index f1232fec7d..540ffb4f0b 100644 --- a/docs/rules/no-runloop.md +++ b/docs/rules/no-runloop.md @@ -12,7 +12,7 @@ This rule disallows usage of `@ember/runloop` functions. Example of **incorrect** code for this rule: -```ts +```js import Component from '@glimmer/component'; import { run } from '@ember/runloop'; @@ -58,7 +58,7 @@ export default class MyComponent extends Component { If you have `@ember/runloop` functions that you wish to allow, you can configure this rule to allow specific methods. The configuration takes an object with the `allowList` property, which is an array of strings where the strings must be names of runloop functions. -```ts +```js module.exports = { rules: { 'ember/no-runloop': [ From 2acec14a570a9864baa1c8e4608dcb4a465d078c Mon Sep 17 00:00:00 2001 From: Lucy Lin Date: Mon, 19 Dec 2022 17:52:18 -0800 Subject: [PATCH 4/4] Fix prefer-rest-params --- docs/rules/no-runloop.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/rules/no-runloop.md b/docs/rules/no-runloop.md index 540ffb4f0b..ea3cb0116a 100644 --- a/docs/rules/no-runloop.md +++ b/docs/rules/no-runloop.md @@ -17,8 +17,8 @@ import Component from '@glimmer/component'; import { run } from '@ember/runloop'; export default class MyComponent extends Component { - constructor() { - super(...arguments); + constructor(...args) { + super(...args); run.later(() => { this.set('date', new Date());