-
-
Notifications
You must be signed in to change notification settings - Fork 212
Post-merge-review: Fix template-no-attrs-in-components: align detection with upstream #2688
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
Changes from 4 commits
8bce8c7
3a86712
11afc7a
5c4bd3e
0d355e2
33446a6
179e8b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,40 +8,87 @@ const ruleTester = new RuleTester({ | |
|
|
||
| ruleTester.run('template-no-attrs-in-components', rule, { | ||
| valid: [ | ||
| '<template>{{@value}}</template>', | ||
| '<template>{{this.value}}</template>', | ||
| // Class component with normal this access | ||
| `import Component from '@glimmer/component'; | ||
| class MyComponent extends Component { | ||
| <template>{{this.args.name}}</template> | ||
| }`, | ||
| // Bare attrs is not accessible without this, so it's allowed | ||
| '<template>{{attrs.value}}</template>', | ||
| '<template>{{attrs}}</template>', | ||
| `import Component from '@glimmer/component'; | ||
| class MyComponent extends Component { | ||
| <template>{{attrs.name}}</template> | ||
| }`, | ||
| // Not a component template path: nothing is flagged, regardless of content. | ||
| { | ||
| filename: 'app/templates/application.hbs', | ||
| code: '<template>{{@value}}</template>', | ||
| }, | ||
| { | ||
| filename: 'app/templates/application.hbs', | ||
| code: '<template>{{this.value}}</template>', | ||
| }, | ||
| // `this.attrs.*` is not a real Ember API, but it is NOT what this rule | ||
| // targets — only bare `attrs.*` is flagged. So outside of a component | ||
| // template, `this.attrs.*` should not be flagged. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should be flagged, unless there is another lint rule that would cover it. This is from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, should be fixed now. The PR was using original.startsWith('attrs.') which misses it. In the Glimmer AST, this.attrs.foo has parts[0] === 'attrs' (this is the receiver, not a part), so switching to parts[0] === 'attrs' catches both forms.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't see any tests with this.attrs -- did I miss them?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| { | ||
| filename: 'app/templates/application.hbs', | ||
| code: '<template>{{this.attrs.foo}}</template>', | ||
| }, | ||
| // Even `attrs.*` itself is only flagged inside component templates. | ||
| { | ||
| filename: 'app/templates/application.hbs', | ||
| code: '<template>{{attrs.value}}</template>', | ||
| }, | ||
| // Inside a component template, non-attrs paths are fine. | ||
| { | ||
| filename: 'app/templates/components/foo.hbs', | ||
| code: '<template>{{@value}}</template>', | ||
| }, | ||
| { | ||
| filename: 'app/templates/components/foo.hbs', | ||
| code: '<template>{{this.value}}</template>', | ||
| }, | ||
| // This rule does NOT flag `this.attrs.*`; only bare `attrs.*`. | ||
| { | ||
| filename: 'app/templates/components/foo.hbs', | ||
| code: '<template>{{this.attrs.foo}}</template>', | ||
| }, | ||
| // Pod-style components path matches the gate, but no `attrs` usage. | ||
| { | ||
| filename: 'app/components/foo/template.hbs', | ||
| code: '<template>{{@value}}</template>', | ||
| }, | ||
| // `-components/` path gate, no `attrs` usage. | ||
| { | ||
| filename: 'app/ui-components/foo.hbs', | ||
| code: '<template>{{@value}}</template>', | ||
| }, | ||
| ], | ||
| invalid: [ | ||
| // Bare `attrs.*` inside `templates/components/` — flagged. | ||
| { | ||
| filename: 'app/templates/components/foo.hbs', | ||
| code: '<template>{{attrs.foo}}</template>', | ||
| output: null, | ||
| errors: [{ messageId: 'noAttrs' }], | ||
| }, | ||
| // Bare `attrs` (no dotted tail) inside `templates/components/` — flagged. | ||
| { | ||
| filename: 'app/templates/components/foo.hbs', | ||
| code: '<template>{{attrs}}</template>', | ||
| output: null, | ||
| errors: [{ messageId: 'noAttrs' }], | ||
| }, | ||
| // Pod-style path `components/*/template` — flagged. | ||
| { | ||
| code: '<template>{{this.attrs.value}}</template>', | ||
| filename: 'app/components/foo/template.hbs', | ||
| code: '<template>{{attrs.name}}</template>', | ||
| output: null, | ||
| errors: [{ messageId: 'noThisAttrs' }], | ||
| errors: [{ messageId: 'noAttrs' }], | ||
| }, | ||
| // `ui/components` path — flagged. | ||
| { | ||
| code: '<template>{{this.attrs}}</template>', | ||
| filename: 'app/ui/components/foo.hbs', | ||
| code: '<template>{{attrs.name}}</template>', | ||
| output: null, | ||
| errors: [{ messageId: 'noThisAttrs' }], | ||
| errors: [{ messageId: 'noAttrs' }], | ||
| }, | ||
| // Class component using this.attrs | ||
| // `-components/` path — flagged. | ||
| { | ||
| code: `import Component from '@glimmer/component'; | ||
| class MyComponent extends Component { | ||
| <template>{{this.attrs.name}}</template> | ||
| }`, | ||
| filename: 'app/ui-components/foo.hbs', | ||
| code: '<template>{{attrs.name}}</template>', | ||
| output: null, | ||
| errors: [{ messageId: 'noThisAttrs' }], | ||
| errors: [{ messageId: 'noAttrs' }], | ||
| }, | ||
| ], | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not hbs only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, fixed