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

Add new rule no-at-ember-render-modifiers #1902

Merged
merged 10 commits into from
Jul 7, 2023
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. | ✅ | | |
Expand Down
54 changes: 54 additions & 0 deletions docs/rules/no-at-ember-render-modifiers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# ember/no-at-ember-render-modifiers

<!-- end auto-generated rule header -->

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:
NullVoxPopuli marked this conversation as resolved.
Show resolved Hide resolved

```hbs
<div
{{did-insert this.doInsertBehavior}}
{{did-update this.doUpdateBehavior @arg1 @arg2}}
{{will-destroy this.doDestroyBehavior}}
>
...
</div>
```

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
NullVoxPopuli marked this conversation as resolved.
Show resolved Hide resolved

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)
37 changes: 37 additions & 0 deletions lib/rules/no-at-ember-render-modifiers.js
Original file line number Diff line number Diff line change
@@ -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',
NullVoxPopuli marked this conversation as resolved.
Show resolved Hide resolved
recommended: false,
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')) {
NullVoxPopuli marked this conversation as resolved.
Show resolved Hide resolved
context.report({
node,
message: ERROR_MESSAGE,
});
}
},
};
},
};
54 changes: 54 additions & 0 deletions tests/lib/rules/no-at-ember-render-modifiers.js
Original file line number Diff line number Diff line change
@@ -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"', ''],
NullVoxPopuli marked this conversation as resolved.
Show resolved Hide resolved
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' }],
},
],
});