From ab26937a43f358cd72aa8829bd5a235d7905b437 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Mon, 26 Jun 2023 18:30:37 -0400 Subject: [PATCH 01/10] Implementation -- needs tests --- lib/rules/no-render-modifiers.js | 37 ++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 lib/rules/no-render-modifiers.js diff --git a/lib/rules/no-render-modifiers.js b/lib/rules/no-render-modifiers.js new file mode 100644 index 0000000000..535331533d --- /dev/null +++ b/lib/rules/no-render-modifiers.js @@ -0,0 +1,37 @@ +'use strict'; + +const ERROR_MESSAGE = 'Do not use @ember/render-modifiers' + +'Instead, use derived data patterns, and/or co-locate destruction via @ember/destroyable'; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + type: 'suggestion', + docs: { + description: 'disallow importing from @ember/render-modifiers', + category: 'Miscellaneous', + recommended: false, + url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-render-modifiers.md', + }, + fixable: null, + schema: [], + }, + + ERROR_MESSAGE, + + create(context) { + return { + ImportDeclaration(node) { + if ( + node.source.value.startsWith('@ember/render-modifiers') + ) { + context.report({ node, message: ERROR_MESSAGE }); + } + }, + }; + }, +}; From 5c643e092e320858ad6104adbd8c27ff4debfa90 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 30 Jun 2023 20:41:36 -0400 Subject: [PATCH 02/10] Rename lint --- ...ers.js => no-at-ember-render-modifiers.js} | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) rename lib/rules/{no-render-modifiers.js => no-at-ember-render-modifiers.js} (63%) diff --git a/lib/rules/no-render-modifiers.js b/lib/rules/no-at-ember-render-modifiers.js similarity index 63% rename from lib/rules/no-render-modifiers.js rename to lib/rules/no-at-ember-render-modifiers.js index 535331533d..b46fe9d218 100644 --- a/lib/rules/no-render-modifiers.js +++ b/lib/rules/no-at-ember-render-modifiers.js @@ -1,7 +1,8 @@ 'use strict'; -const ERROR_MESSAGE = 'Do not use @ember/render-modifiers' - +'Instead, use derived data patterns, and/or co-locate destruction via @ember/destroyable'; +const error = 'Do not use @ember/render-modifiers.'; +const help = + 'Instead, use derived data patterns, and/or co-locate destruction via @ember/destroyable'; //------------------------------------------------------------------------------ // Rule Definition @@ -21,15 +22,18 @@ module.exports = { schema: [], }, - ERROR_MESSAGE, - create(context) { return { ImportDeclaration(node) { - if ( - node.source.value.startsWith('@ember/render-modifiers') - ) { - context.report({ node, message: ERROR_MESSAGE }); + if (node.source.value.startsWith('@ember/render-modifiers')) { + context.report({ + node, + message: '{{error}} {{help}}', + data: { + error, + help, + }, + }); } }, }; From 779dc01a6811b03c4dd01725bbbf4f04004ded4c Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 30 Jun 2023 21:12:16 -0400 Subject: [PATCH 03/10] Add tests, run update commands --- README.md | 1 + docs/rules/no-at-ember-render-modifiers.md | 7 +++ lib/rules/no-at-ember-render-modifiers.js | 16 +++--- .../lib/rules/no-at-ember-render-modifiers.js | 54 +++++++++++++++++++ 4 files changed, 68 insertions(+), 10 deletions(-) create mode 100644 docs/rules/no-at-ember-render-modifiers.md create mode 100644 tests/lib/rules/no-at-ember-render-modifiers.js diff --git a/README.md b/README.md index 091945bf97..6913d38ca1 100644 --- a/README.md +++ b/README.md @@ -154,6 +154,7 @@ module.exports = { | Name                                               | Description | 💼 | 🔧 | 💡 | | :--------------------------------------------------------------------------------------------------------------------- | :---------------------------------------------------------------------------------------------------------------------------- | :- | :- | :- | | [named-functions-in-promises](docs/rules/named-functions-in-promises.md) | enforce usage of named functions in promises | | | | +| [no-at-ember-render-modifiers](docs/rules/no-at-ember-render-modifiers.md) | disallow importing from @ember/render-modifiers | | | | | [no-html-safe](docs/rules/no-html-safe.md) | disallow the use of `htmlSafe` | | | | | [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. | ✅ | | | diff --git a/docs/rules/no-at-ember-render-modifiers.md b/docs/rules/no-at-ember-render-modifiers.md new file mode 100644 index 0000000000..a9dcd549e3 --- /dev/null +++ b/docs/rules/no-at-ember-render-modifiers.md @@ -0,0 +1,7 @@ +# ember/no-at-ember-render-modifiers + + + +## Examples + +## References diff --git a/lib/rules/no-at-ember-render-modifiers.js b/lib/rules/no-at-ember-render-modifiers.js index b46fe9d218..62540716b1 100644 --- a/lib/rules/no-at-ember-render-modifiers.js +++ b/lib/rules/no-at-ember-render-modifiers.js @@ -1,9 +1,7 @@ 'use strict'; -const error = 'Do not use @ember/render-modifiers.'; -const help = - 'Instead, use derived data patterns, and/or co-locate destruction via @ember/destroyable'; - +const ERROR_MESSAGE = + 'Do not use @ember/render-modifiers. Instead, use derived data patterns, and/or co-locate destruction via @ember/destroyable'; //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -16,23 +14,21 @@ module.exports = { description: 'disallow importing from @ember/render-modifiers', category: 'Miscellaneous', recommended: false, - url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-render-modifiers.md', + url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-at-ember-render-modifiers.md', }, fixable: null, schema: [], }, + ERROR_MESSAGE, + create(context) { return { ImportDeclaration(node) { if (node.source.value.startsWith('@ember/render-modifiers')) { context.report({ node, - message: '{{error}} {{help}}', - data: { - error, - help, - }, + message: ERROR_MESSAGE, }); } }, diff --git a/tests/lib/rules/no-at-ember-render-modifiers.js b/tests/lib/rules/no-at-ember-render-modifiers.js new file mode 100644 index 0000000000..e85070366f --- /dev/null +++ b/tests/lib/rules/no-at-ember-render-modifiers.js @@ -0,0 +1,54 @@ +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/no-at-ember-render-modifiers'); +const RuleTester = require('eslint').RuleTester; + +const { ERROR_MESSAGE } = rule; + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 2020, + sourceType: 'module', + }, +}); +ruleTester.run('no-at-ember-render-modifiers', rule, { + valid: ['import x from "x"', ''], + invalid: [ + { + code: 'import "@ember/render-modifiers";', + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'ImportDeclaration' }], + }, + { + code: 'import x from "@ember/render-modifiers";', + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'ImportDeclaration' }], + }, + { + code: 'import { x } from "@ember/render-modifiers";', + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'ImportDeclaration' }], + }, + { + code: 'import didInsert from "@ember/render-modifiers/modifiers/did-insert";', + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'ImportDeclaration' }], + }, + { + code: 'import didUpdate from "@ember/render-modifiers/modifiers/did-update";', + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'ImportDeclaration' }], + }, + { + code: 'import willDestroy from "@ember/render-modifiers/modifiers/will-destroy";', + output: null, + errors: [{ message: ERROR_MESSAGE, type: 'ImportDeclaration' }], + }, + ], +}); From 50b2c6db80236aae5e45a637a5b28f4d4bce541d Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 30 Jun 2023 21:38:57 -0400 Subject: [PATCH 04/10] Add docs --- docs/rules/no-at-ember-render-modifiers.md | 44 ++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/docs/rules/no-at-ember-render-modifiers.md b/docs/rules/no-at-ember-render-modifiers.md index a9dcd549e3..5b0ad5e699 100644 --- a/docs/rules/no-at-ember-render-modifiers.md +++ b/docs/rules/no-at-ember-render-modifiers.md @@ -2,6 +2,50 @@ +What's wrong with `{{did-insert}}`, `{{did-update}}`, and `{{will-destroy}}`? + +These modifiers were meant for temporary migration purposes for quickly migrating `@ember/component` from before Octane to the `@glimmer/component` we have today. Since `@ember/compoennt` implicitly had a wrapping `div` around each component and `@glimmer/compoennt`s have "outer HTML" semantics, an automated migration could have ended up looking something like: +```hbs +
+ ... +
+``` + +It was intended that this would be a temporary step to help get folks off of `@ember/components` quickly in early 2020 when folks migrated to the Octane editon, but some folks continued using these modifiers. + +Additionally, this style of mananging data flow has some flaws: +- an element is required + - this can be mitigated by using helpers, but they have the same problems mentioned below +- the behavior that is used with these modifiers can cause extra renders and infinite rendering loops + - this is the nature of "effect"-driven development / data-flow, every time an effect runs, rendering must re-occur. +- behavior that needs to be co-located is spread out, making maintenance and debugging harder + - each part of the responsibility of a "behavior" or "feature" is spread out, making it harder to find and comprehend the full picture of that behavior or feature. + + ## Examples +This rule **forbids** the following: + +```js +import didInsert from '@ember/render-modifiers/modifiers/did-insert'; +``` +```js +import didUpdate from '@ember/render-modifiers/modifiers/did-update'; +``` +```js +import willDestroy from '@ember/render-modifiers/modifiers/will-destroy'; +``` + ## References + +- [Editions](https://emberjs.com/editions/) +- [Octane Upgrade Guide](https://guides.emberjs.com/release/upgrading/current-edition/) +- [Component Documentation](https://guides.emberjs.com/release/components/) +- [Avoiding Lifecycle in Component](https://nullvoxpopuli.com/avoiding-lifecycle) +- [The `ember-template-lint` version of this rule](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-at-ember-render-modifiers.md) +- [`ember-modifier`](https://github.com/ember-modifier/ember-modifier) +- [`@ember/render-modifiers`](https://github.com/emberjs/ember-render-modifiers) (deprecated) From e7f9cd8b0f0c4c158d200a2137f661c1f69d2901 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 30 Jun 2023 22:08:31 -0400 Subject: [PATCH 05/10] Lint docs --- docs/rules/no-at-ember-render-modifiers.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/rules/no-at-ember-render-modifiers.md b/docs/rules/no-at-ember-render-modifiers.md index 5b0ad5e699..781d8582bd 100644 --- a/docs/rules/no-at-ember-render-modifiers.md +++ b/docs/rules/no-at-ember-render-modifiers.md @@ -5,6 +5,7 @@ What's wrong with `{{did-insert}}`, `{{did-update}}`, and `{{will-destroy}}`? These modifiers were meant for temporary migration purposes for quickly migrating `@ember/component` from before Octane to the `@glimmer/component` we have today. Since `@ember/compoennt` implicitly had a wrapping `div` around each component and `@glimmer/compoennt`s have "outer HTML" semantics, an automated migration could have ended up looking something like: + ```hbs
Date: Thu, 6 Jul 2023 15:13:44 -0400 Subject: [PATCH 06/10] Update docs/rules/no-at-ember-render-modifiers.md Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com> --- docs/rules/no-at-ember-render-modifiers.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-at-ember-render-modifiers.md b/docs/rules/no-at-ember-render-modifiers.md index 781d8582bd..c38cad027e 100644 --- a/docs/rules/no-at-ember-render-modifiers.md +++ b/docs/rules/no-at-ember-render-modifiers.md @@ -4,7 +4,7 @@ What's wrong with `{{did-insert}}`, `{{did-update}}`, and `{{will-destroy}}`? -These modifiers were meant for temporary migration purposes for quickly migrating `@ember/component` from before Octane to the `@glimmer/component` we have today. Since `@ember/compoennt` implicitly had a wrapping `div` around each component and `@glimmer/compoennt`s have "outer HTML" semantics, an automated migration could have ended up looking something like: +These modifiers were meant for temporary migration purposes for quickly migrating `@ember/component` from before Octane to the `@glimmer/component` we have today. Since `@ember/component` implicitly had a wrapping `div` around each component and `@glimmer/component`s have "outer HTML" semantics, an automated migration could have ended up looking something like: ```hbs
Date: Thu, 6 Jul 2023 20:33:08 -0400 Subject: [PATCH 07/10] Use more specific import path check --- lib/rules/no-at-ember-render-modifiers.js | 6 +++++- tests/lib/rules/no-at-ember-render-modifiers.js | 8 +++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/rules/no-at-ember-render-modifiers.js b/lib/rules/no-at-ember-render-modifiers.js index 62540716b1..4e978d4ad8 100644 --- a/lib/rules/no-at-ember-render-modifiers.js +++ b/lib/rules/no-at-ember-render-modifiers.js @@ -6,6 +6,10 @@ const ERROR_MESSAGE = // Rule Definition //------------------------------------------------------------------------------ +function importDeclarationIsPackageName(node, path) { + return node.source.value === path || node.source.value.startsWith(`${path}/`); +} + /** @type {import('eslint').Rule.RuleModule} */ module.exports = { meta: { @@ -25,7 +29,7 @@ module.exports = { create(context) { return { ImportDeclaration(node) { - if (node.source.value.startsWith('@ember/render-modifiers')) { + if (importDeclarationIsPackageName(node, '@ember/render-modifiers')) { context.report({ node, message: ERROR_MESSAGE, diff --git a/tests/lib/rules/no-at-ember-render-modifiers.js b/tests/lib/rules/no-at-ember-render-modifiers.js index e85070366f..2a7b093156 100644 --- a/tests/lib/rules/no-at-ember-render-modifiers.js +++ b/tests/lib/rules/no-at-ember-render-modifiers.js @@ -18,7 +18,13 @@ const ruleTester = new RuleTester({ }, }); ruleTester.run('no-at-ember-render-modifiers', rule, { - valid: ['import x from "x"', ''], + valid: [ + 'import x from "x"', + '', + "import { x } from 'foo';", + "import x from '@ember/foo';", + "import x from '@ember/render-modifiers-foo';", + ], invalid: [ { code: 'import "@ember/render-modifiers";', From 80ed74e046e234f75e7a5783423dd6895be60ba9 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Thu, 6 Jul 2023 20:33:52 -0400 Subject: [PATCH 08/10] Change category to Deprecations --- lib/rules/no-at-ember-render-modifiers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/no-at-ember-render-modifiers.js b/lib/rules/no-at-ember-render-modifiers.js index 4e978d4ad8..d3db3ce9e5 100644 --- a/lib/rules/no-at-ember-render-modifiers.js +++ b/lib/rules/no-at-ember-render-modifiers.js @@ -16,7 +16,7 @@ module.exports = { type: 'suggestion', docs: { description: 'disallow importing from @ember/render-modifiers', - category: 'Miscellaneous', + category: 'Deprecations', recommended: false, url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-at-ember-render-modifiers.md', }, From 91aa580744510aa63601df4ace9068baa85934be Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Thu, 6 Jul 2023 20:35:43 -0400 Subject: [PATCH 09/10] Add link to ember-template-lint --- docs/rules/no-at-ember-render-modifiers.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/rules/no-at-ember-render-modifiers.md b/docs/rules/no-at-ember-render-modifiers.md index c38cad027e..11c8131a23 100644 --- a/docs/rules/no-at-ember-render-modifiers.md +++ b/docs/rules/no-at-ember-render-modifiers.md @@ -43,6 +43,8 @@ import didUpdate from '@ember/render-modifiers/modifiers/did-update'; import willDestroy from '@ember/render-modifiers/modifiers/will-destroy'; ``` +For more examples, see [the docs on ember-template-lint](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-at-ember-render-modifiers.md). + ## References - [Editions](https://emberjs.com/editions/) From 476c91da287cafd5844f44feebf247c84abba6ec Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 7 Jul 2023 08:13:28 -0400 Subject: [PATCH 10/10] Run update script --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 6913d38ca1..b73f8f9a16 100644 --- a/README.md +++ b/README.md @@ -101,6 +101,7 @@ module.exports = { | [closure-actions](docs/rules/closure-actions.md) | enforce usage of closure actions | ✅ | | | | [new-module-imports](docs/rules/new-module-imports.md) | enforce using "New Module Imports" from Ember RFC #176 | ✅ | | | | [no-array-prototype-extensions](docs/rules/no-array-prototype-extensions.md) | disallow usage of Ember's `Array` prototype extensions | | 🔧 | | +| [no-at-ember-render-modifiers](docs/rules/no-at-ember-render-modifiers.md) | disallow importing from @ember/render-modifiers | | | | | [no-deprecated-router-transition-methods](docs/rules/no-deprecated-router-transition-methods.md) | enforce usage of router service transition methods | | 🔧 | | | [no-function-prototype-extensions](docs/rules/no-function-prototype-extensions.md) | disallow usage of Ember's `function` prototype extensions | ✅ | | | | [no-implicit-injections](docs/rules/no-implicit-injections.md) | enforce usage of implicit service injections | | 🔧 | | @@ -154,7 +155,6 @@ module.exports = { | Name                                               | Description | 💼 | 🔧 | 💡 | | :--------------------------------------------------------------------------------------------------------------------- | :---------------------------------------------------------------------------------------------------------------------------- | :- | :- | :- | | [named-functions-in-promises](docs/rules/named-functions-in-promises.md) | enforce usage of named functions in promises | | | | -| [no-at-ember-render-modifiers](docs/rules/no-at-ember-render-modifiers.md) | disallow importing from @ember/render-modifiers | | | | | [no-html-safe](docs/rules/no-html-safe.md) | disallow the use of `htmlSafe` | | | | | [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. | ✅ | | |