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 #2554

Merged
51 changes: 51 additions & 0 deletions docs/rule/no-at-ember-render-modifiers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# no-at-ember-render-modifiers

`@ember/render-modifiers` were meant as a transitional tool from the pre-Octane era, and not long-term usage.

More information about this can be found on the [`@ember/render-modifiers` README](https://github.com/emberjs/ember-render-modifiers#when-to-use-these-modifiers-and-when-not-to-use-them).

## Examples

This rule **forbids** the following:

```hbs
<div {{did-insert this.someFunction}}>
```

```hbs
<div {{did-update this.someFunction}}>
```

```hbs
<div {{will-destroy this.someFunction}}>
```

## Migration

The migration path typically depends on what the render-modifier was used for, but if you need a custom modifier, the [`ember-modifier` README](https://github.com/ember-modifier/ember-modifier) covers everything you need to know for making custom modifiers.

For example, if render modifiers were used for setup/teardown, the migration to `ember-modifier` could be the following:

```js
NullVoxPopuli marked this conversation as resolved.
Show resolved Hide resolved
import Component from '@glimmer/component';
import { modifier } from 'ember-modifier';

export default class MyComponent extends Component {
myModifier = modifier((element) => {
let handleEvent = () => {};

element.addEventListener('eventName', handleEvent);

return () => element.removeEventListener('eventName', handelEvent);
});
}
```

```hbs
<div {{this.myModifier}}>
```
Comment on lines +44 to +46
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this code sample for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the template for the above class


## References

- [`@ember/render-modifiers`](https://github.com/emberjs/ember-render-modifiers) (deprecated)
- [`ember-modifier`](https://github.com/ember-modifier/ember-modifier)
2 changes: 2 additions & 0 deletions lib/rules/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import noargumentsforhtmlelements from './no-arguments-for-html-elements.js';
import noariahiddenbody from './no-aria-hidden-body.js';
import noariaunsupportedelements from './no-aria-unsupported-elements.js';
import noarrayprototypeextensions from './no-array-prototype-extensions.js';
import noatemberrendermodifiers from './no-at-ember-render-modifiers.js';
import noattrsincomponents from './no-attrs-in-components.js';
import noautofocusattribute from './no-autofocus-attribute.js';
import nobarestrings from './no-bare-strings.js';
Expand Down Expand Up @@ -140,6 +141,7 @@ export default {
'no-aria-hidden-body': noariahiddenbody,
'no-aria-unsupported-elements': noariaunsupportedelements,
'no-array-prototype-extensions': noarrayprototypeextensions,
'no-at-ember-render-modifiers': noatemberrendermodifiers,
'no-attrs-in-components': noattrsincomponents,
'no-autofocus-attribute': noautofocusattribute,
'no-bare-strings': nobarestrings,
Expand Down
30 changes: 30 additions & 0 deletions lib/rules/no-at-ember-render-modifiers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import Rule from './_base.js';

const WHY = `This modifier was intended to ease migration to Octane and not for long-term side-effects.`;
const ACTION = `Instead, refactor to use a custom modifier. See https://github.com/ember-modifier/ember-modifier`;
export const MESSAGES = {
'did-insert': `Do not use the \`did-insert\` modifier. ${WHY} ${ACTION}`,
'did-update': `Do not use the \`did-update\` modifier. ${WHY} ${ACTION}`,
'will-destroy': `Do not use the \`will-destroy\` modifier. ${WHY} ${ACTION}`,
};

const MODIFIERS = new Set([`did-insert`, `did-update`, `will-destroy`]);

export default class NoAtEmberRenderModifiers extends Rule {
visitor() {
return {
ElementModifierStatement(node) {
let modifierName = node.path.original;
if (!MODIFIERS.has(modifierName)) {
return;
}

this.log({
message: MESSAGES[modifierName],
node,
source: this.sourceForNode(node),
});
},
};
}
}
139 changes: 139 additions & 0 deletions test/unit/rules/no-at-ember-render-modifiers-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
import { MESSAGES } from '../../../lib/rules/no-at-ember-render-modifiers.js';
import generateRuleTests from '../../helpers/rule-test-harness.js';

generateRuleTests({
name: 'no-at-ember-render-modifiers',

config: true,

good: [
'<div {{this.someModifier}}></div>',
'<div {{someModifier}}></div>',
'<div {{did-foo}}></div>',
// helper -- a different rule should prevent this
// https://github.com/buschtoens/ember-render-helpers (depending on usage)
'{{did-insert}}',
'{{did-update}}',
'{{will-destroy}}',
],

bad: [
{
template: '<div {{did-insert}}></div>',
verifyResults(results) {
expect({ results }).toMatchInlineSnapshot(
{
results: [
{
column: 5,
endColumn: 19,
endLine: 1,
filePath: 'layout.hbs',
line: 1,
message: MESSAGES['did-insert'],
rule: 'no-at-ember-render-modifiers',
severity: 2,
source: '{{did-insert}}',
},
],
},
`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's some old leftover snapshots in these tests. There should only be one arg to toMatchInlineSnapshot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the lint prevented from using a string, so I had to change to a real object.
then, when jest was running, it updated the string stuff after success -- the string part is the real snapshot persisted by jest, and the js object is the "expected" and helps out with the diff and generated the snapshot if the actual matches the expected.

Is this not how it's supposed to work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jest's auto-updating of inline snapshots is buggy. It left the old snapshot in there. Just clear out the snapshots and let it regenerate without the extra arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't look buggy to me -- what do you mean?
looks up to date.
are you looking at the full PR diff, rather than the commit this thread started on? 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two arguments passed to toMatchInlineSnapshot in your code. There should only be one. One is a duplicate of the other. You need to delete the arguments to toMatchInlineSnapshot and re-run jest to regenerate the snapshots.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if I do that, jest adds the second argument back -- no diff is seen when doing git status / git diff.

To prove, I've added two commits:

  • first removes the second argument, as you suggest
  • second, is what happens after running the tests again

diff:

❯ git diff HEAD~2

ember-template-lint on  no-at-ember-render-modifiers [⇡] 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing both args was necessary before regenerating. I just pushed the fix for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! tho, now I don't know why the lint rule I was seeing isn't triggering 😅
(It was a jest one, and why what happened even happened in the first place)

{
"results": [
{
"column": 5,
"endColumn": 19,
"endLine": 1,
"filePath": "layout.hbs",
"line": 1,
"message": "Do not use the \`did-insert\` modifier. This modifier was intended to ease migration to Octane and not for long-term side-effects. Instead, refactor to use a custom modifier. See https://github.com/ember-modifier/ember-modifier",
"rule": "no-at-ember-render-modifiers",
"severity": 2,
"source": "{{did-insert}}",
},
],
}
`
);
},
},
{
template: '<div {{did-update}}></div>',
verifyResults(results) {
expect({ results }).toMatchInlineSnapshot(
{
results: [
{
column: 5,
endColumn: 19,
endLine: 1,
filePath: 'layout.hbs',
line: 1,
message: MESSAGES['did-update'],
rule: 'no-at-ember-render-modifiers',
severity: 2,
source: '{{did-update}}',
},
],
},
`
{
"results": [
{
"column": 5,
"endColumn": 19,
"endLine": 1,
"filePath": "layout.hbs",
"line": 1,
"message": "Do not use the \`did-update\` modifier. This modifier was intended to ease migration to Octane and not for long-term side-effects. Instead, refactor to use a custom modifier. See https://github.com/ember-modifier/ember-modifier",
"rule": "no-at-ember-render-modifiers",
"severity": 2,
"source": "{{did-update}}",
},
],
}
`
);
},
},
{
template: '<div {{will-destroy}}></div>',
verifyResults(results) {
expect({ results }).toMatchInlineSnapshot(
{
results: [
{
column: 5,
endColumn: 21,
endLine: 1,
filePath: 'layout.hbs',
line: 1,
message: MESSAGES['will-destroy'],
rule: 'no-at-ember-render-modifiers',
severity: 2,
source: '{{will-destroy}}',
},
],
},
`
{
"results": [
{
"column": 5,
"endColumn": 21,
"endLine": 1,
"filePath": "layout.hbs",
"line": 1,
"message": "Do not use the \`will-destroy\` modifier. This modifier was intended to ease migration to Octane and not for long-term side-effects. Instead, refactor to use a custom modifier. See https://github.com/ember-modifier/ember-modifier",
"rule": "no-at-ember-render-modifiers",
"severity": 2,
"source": "{{will-destroy}}",
},
],
}
`
);
},
},
],
});