Add new rule no-tracked-properties-from-args
#1702
Conversation
| /** @type {import('eslint').Rule.RuleModule} */ | ||
| module.exports = { | ||
| meta: { | ||
| type: 'suggestion', |
There was a problem hiding this comment.
If this issue will cause a bug then we can use problem instead of suggestion.
| }); | ||
|
|
||
| ruleTester.run('no-tracked-properties-from-args', rule, { | ||
| valid: [ |
There was a problem hiding this comment.
Another valid test case: @tracked test = this.notArgs.args.test
| schema: [], | ||
| }, | ||
|
|
||
| ERROR_MESSAGE, |
There was a problem hiding this comment.
There's a new and better way to define the message: https://github.com/eslint-community/eslint-plugin-eslint-plugin/blob/main/docs/rules/prefer-message-ids.md
| @tracked | ||
| test = this.args.test; |
There was a problem hiding this comment.
Is this a whitespace-only difference from the previous test case? If the AST is the same (astexplorer.net), then no need for a separate test case.
| type: 'suggestion', | ||
| docs: { | ||
| description: 'disallow creating @tracked properties from this.args', | ||
| category: 'Ember Decorator', |
There was a problem hiding this comment.
Let's just put this under Ember Octane with the other decorator-related rules.
|
|
||
| ## Examples | ||
|
|
||
| Examples of **incorrect** code for this rule: |
There was a problem hiding this comment.
Can you show a correct example? (with a getter?)
bmish
changed the title
no-tracked-properties-from-args
|
When creating a PR, you need to use the syntax |
| return { | ||
| PropertyDefinition(node) { | ||
| const hasTrackedDecorator = | ||
| node.decorators?.length > 0 && node.decorators[0].expression.name === 'tracked'; |
There was a problem hiding this comment.
Can we check against the imported name of tracked instead of hardcoding it? It will be a little more robust in case someone does:
import { tracked as foobar } from '@glimmer/tracking';
...
@foobar foo = this.args.bar;
|
|
||
| create(context) { | ||
| return { | ||
| PropertyDefinition(node) { |
There was a problem hiding this comment.
Do we still need to support ClassProperty as well? @bmish
There was a problem hiding this comment.
It's not a bad idea to support that too until we drop support for ESLint v7. Context: #1529
Although I'm not super worried about it if it's a hassle...
There was a problem hiding this comment.
Thanks! Borrowing the implementation from no-private-routing-service.js, but is there a way to have the tests run against both of these versions?
There was a problem hiding this comment.
CI runs tests against both ESLint v7 and ESLint v8 so as long as it passes, we should be good.
README.md
Outdated
| @@ -115,6 +115,12 @@ module.exports = { | |||
| | [no-empty-attrs](docs/rules/no-empty-attrs.md) | disallow usage of empty attributes in Ember Data models | | | | | |||
| | [use-ember-data-rfc-395-imports](docs/rules/use-ember-data-rfc-395-imports.md) | enforce usage of `@ember-data/` package imports instead `ember-data` | ✅ | 🔧 | | | |||
|
|
|||
| ### Ember Decorator | |||
|
|
||
| ## Rule Details | ||
|
|
||
| This rule disallows the creation of @tracked properties with values from `this.args`. The @tracked property will not be updated when the args change, which is almost never what you want. |
There was a problem hiding this comment.
Can you add a paragraph or at least a sentence explaining how to fix the issue?
| Examples of **correct** code for this rule: | ||
| ```js | ||
| get someValue() { | ||
| return this.args.someValue; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Does adding a getter for the arg actually address the user's original intent in using @tracked? Does it mean that @tracked was entirely unnecessary?
There was a problem hiding this comment.
I would still like to understand this a bit better, especially as to whether a getter is really a complete fix for what the user is attempting to achieve? We don't have to show a complete fix necessarily, but I would like to make sure we describe at a high-level what kind of more-involved fixes might be necessary.
There was a problem hiding this comment.
For the simple use case of trying to rename a deeply nested arg, @Tracked would be unnecessary and it would just be used interchangeably with a getter. Trying to raise awareness about the extra pitfall here.
I added a note on having to mutate the args to the docs portion, let me know if that helps!
|
Do you think this should become a |
I would say so, having to debug two sources of truth is usually a hard to debug case. |
Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
|
Excited to have you as a contributor! |
|
Excited as well - thanks for contributing a great rule :D |
Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com> Co-authored-by: Joan Cejudo <joan@joans-mbp.myfiosgateway.com> Co-authored-by: Joan Cejudo <joan@Joans-MacBook-Pro.local> Fixes ember-cli#1701
Fixes #1701