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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Jun 27, 2023
Merged

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

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4a659ac
Add no-@ember/render-modifiers
NullVoxPopuli Jul 14, 2022
d01b765
Update tests
NullVoxPopuli Jul 16, 2022
8f568a4
Apply suggestions from code review
lin-ll May 24, 2023
069fd65
Update tests
NullVoxPopuli Jun 26, 2023
eb596d6
Use modifier node instead of element node
NullVoxPopuli Jun 26, 2023
32a257d
Update docs
NullVoxPopuli Jun 26, 2023
b0a5096
Update readme
NullVoxPopuli Jun 27, 2023
d3acdaa
Remove second argument to matchInlineSnapshot
NullVoxPopuli Jun 27, 2023
5b99d5b
Run jest in watch mode, commit changes
NullVoxPopuli Jun 27, 2023
c9b161b
fix snapshots
bmish Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
158 changes: 112 additions & 46 deletions test/unit/rules/no-at-ember-render-modifiers-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,67 +6,133 @@ generateRuleTests({

config: true,

good: ['<div {{this.someModifier}}></div>', '<div {{someModifier}}></div>'],
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(`
[
{
"column": 5,
"endColumn": 19,
"endLine": 1,
"filePath": "layout.hbs",
"line": 1,
"message": "${MESSAGES['did-insert']}",
"rule": "no-at-ember-render-modifiers",
"severity": 2,
"source": "<div {{did-insert}}></div>",
},
]
`);
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: '<div {{did-insert}}></div>',
},
],
},
`
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": "<div {{did-insert}}></div>",
},
],
}
`
);
},
},
{
template: '<div {{did-update}}></div>',
verifyResults(results) {
expect(results).toMatchInlineSnapshot(`
[
{
"column": 5,
"endColumn": 19,
"endLine": 1,
"filePath": "layout.hbs",
"line": 1,
"message": "${MESSAGES['did-update']}",
"rule": "no-at-ember-render-modifiers",
"severity": 2,
"source": "<div {{did-update}}></div>",
},
]
`);
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: '<div {{did-update}}></div>',
},
],
},
`
{
"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": "<div {{did-update}}></div>",
},
],
}
`
);
},
},
{
template: '<div {{will-destroy}}></div>',
verifyResults(results) {
expect(results).toMatchInlineSnapshot(`
[
{
"column": 5,
"endColumn": 21,
"endLine": 1,
"filePath": "layout.hbs",
"line": 1,
"message": "${MESSAGES['will-destroy']}",
"rule": "no-at-ember-render-modifiers",
"severity": 2,
"source": "<div {{will-destroy}}></div>",
},
]
`);
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: '<div {{will-destroy}}></div>',
},
],
},
`
{
"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": "<div {{will-destroy}}></div>",
},
],
}
`
);
},
},
],
Expand Down