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-tracked-properties-from-args
#1702
Changes from 5 commits
180d08e
395ae63
70e312c
04622b5
634a15e
1d9bdf0
a33ff58
7615023
ded063f
5f6b579
1f2d52c
f3bb1b9
5cf5659
d049e2a
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 |
---|---|---|
@@ -0,0 +1,29 @@ | ||
# ember/no-tracked-properties-from-args | ||
|
||
<!-- end auto-generated rule header --> | ||
|
||
Disallow creation of @tracked properties from args. | ||
|
||
## 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a paragraph or at least a sentence explaining how to fix the issue? |
||
|
||
## Examples | ||
|
||
Examples of **incorrect** code for this rule: | ||
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. Can you show a correct example? (with a getter?) |
||
|
||
```js | ||
@tracked someValue = this.args.someValue; | ||
``` | ||
|
||
Examples of **correct** code for this rule: | ||
```js | ||
joancc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
get someValue() { | ||
return this.args.someValue; | ||
} | ||
``` | ||
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. Does adding a getter for the arg actually address the user's original intent in using 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
## References | ||
|
||
- [Glimmer Components - args](https://guides.emberjs.com/release/upgrading/current-edition/glimmer-components/#toc_getting-used-to-glimmer-components) guide | ||
- [tracked](https://guides.emberjs.com/release/in-depth-topics/autotracking-in-depth/) guide |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
'use strict'; | ||
|
||
const { getImportIdentifier } = require('../utils/import'); | ||
|
||
const ERROR_MESSAGE = | ||
'Do not use this.args to create @tracked properties as this will not be updated if the args change. Instead use a getter.'; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Rule Definition | ||
//------------------------------------------------------------------------------ | ||
|
||
/** @type {import('eslint').Rule.RuleModule} */ | ||
module.exports = { | ||
meta: { | ||
type: 'problem', | ||
docs: { | ||
description: 'disallow creating @tracked properties from this.args', | ||
category: 'Ember Octane', | ||
recommended: false, | ||
url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-tracked-properties-from-args.md', | ||
}, | ||
fixable: null, | ||
schema: [], | ||
messages: { | ||
main: ERROR_MESSAGE, | ||
}, | ||
}, | ||
create(context) { | ||
let trackedImportName; | ||
|
||
return { | ||
ImportDeclaration(node) { | ||
if (node.source.value === '@glimmer/tracking') { | ||
trackedImportName = getImportIdentifier(node, '@glimmer/tracking', 'tracked'); | ||
bmish marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}, | ||
PropertyDefinition(node) { | ||
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. Do we still need to support 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'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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CI runs tests against both ESLint v7 and ESLint v8 so as long as it passes, we should be good. |
||
const hasTrackedDecorator = | ||
node.decorators?.length > 0 && node.decorators[0].expression.name === trackedImportName; | ||
const hasThisArgsValue = | ||
node.value.object?.type === 'MemberExpression' && | ||
node.value.object?.object.type === 'ThisExpression' && | ||
node.value.object?.property.name === 'args'; | ||
|
||
if (hasTrackedDecorator && hasThisArgsValue) { | ||
context.report({ node, messageId: 'main' }); | ||
} | ||
}, | ||
}; | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
//------------------------------------------------------------------------------ | ||
// Requirements | ||
//------------------------------------------------------------------------------ | ||
|
||
const rule = require('../../../lib/rules/no-tracked-properties-from-args'); | ||
const RuleTester = require('eslint').RuleTester; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Tests | ||
//------------------------------------------------------------------------------ | ||
|
||
const ruleTester = new RuleTester({ | ||
parser: require.resolve('@babel/eslint-parser'), | ||
parserOptions: { | ||
ecmaVersion: 2020, | ||
sourceType: 'module', | ||
}, | ||
}); | ||
|
||
ruleTester.run('no-tracked-properties-from-args', rule, { | ||
valid: [ | ||
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. Another valid test case: |
||
` | ||
import { tracked } from '@glimmer/tracking' | ||
|
||
class Test { | ||
@someDecorator test = this.args.test | ||
}`, | ||
` | ||
import { tracked } from '@glimmer/tracking' | ||
|
||
class Test { | ||
@tracked test = this.someValue | ||
}`, | ||
` | ||
import { tracked } from '@glimmer/tracking' | ||
|
||
class Test { | ||
@tracked test = 7 | ||
}`, | ||
` | ||
import { tracked } from '@glimmer/tracking' | ||
|
||
class Test { | ||
test = 7 | ||
}`, | ||
` | ||
import { tracked } from '@glimmer/tracking' | ||
|
||
class Test { | ||
test = "test" | ||
}`, | ||
` | ||
import { tracked } from '@glimmer/tracking' | ||
|
||
class Test { | ||
@tracked test = this.notArgs.args.test | ||
}`, | ||
` | ||
import { tracked as fooTracked } from '@glimmer/tracking'; | ||
|
||
class Test { | ||
@fooTracked test = this.notArgs.args.test | ||
} | ||
`, | ||
], | ||
invalid: [ | ||
{ | ||
code: ` | ||
import { tracked } from '@glimmer/tracking' | ||
|
||
class Test { | ||
@tracked test = this.args.test; | ||
}`, | ||
output: null, | ||
errors: [ | ||
{ | ||
messageId: 'main', | ||
type: 'PropertyDefinition', | ||
bmish marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
import { tracked as fooTracked } from '@glimmer/tracking'; | ||
|
||
class Test { | ||
@fooTracked test = this.args.test | ||
} | ||
`, | ||
output: null, | ||
errors: [ | ||
{ | ||
messageId: 'main', | ||
type: 'PropertyDefinition', | ||
bmish marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
], | ||
}, | ||
], | ||
}); |
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.
need to re-run the doc generator