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-tracked-properties-from-args #1702

Merged
merged 14 commits into from Dec 19, 2022
1 change: 1 addition & 0 deletions README.md
Expand Up @@ -137,6 +137,7 @@ module.exports = {
| [no-classic-classes](docs/rules/no-classic-classes.md) | disallow "classic" classes in favor of native JS classes | ✅ | | |
| [no-ember-super-in-es-classes](docs/rules/no-ember-super-in-es-classes.md) | disallow use of `this._super` in ES class methods | ✅ | 🔧 | |
| [no-empty-glimmer-component-classes](docs/rules/no-empty-glimmer-component-classes.md) | disallow empty backing classes for Glimmer components | ✅ | | |
| [no-tracked-properties-from-args](docs/rules/no-tracked-properties-from-args.md) | disallow creating @tracked properties from this.args | | | |

### jQuery

Expand Down
34 changes: 34 additions & 0 deletions docs/rules/no-tracked-properties-from-args.md
@@ -0,0 +1,34 @@
# 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. Instead, use a getter to derive the desired state.
If you need to modify a specific arg, consider having the parent provide a way for the child component to update it. This avoids having two sources of truth that you will need to keep in sync.

## Examples

Examples of **incorrect** code for this rule:
Copy link
Member

Choose a reason for hiding this comment

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

Can you show a correct example? (with a getter?)


```js
class Example {
@tracked someValue = this.args.someValue;
}
```

Examples of **correct** code for this rule:
```js
joancc marked this conversation as resolved.
Show resolved Hide resolved
class Example {
get someValue() {
return this.args.someValue;
}
}
```

## 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
61 changes: 61 additions & 0 deletions lib/rules/no-tracked-properties-from-args.js
@@ -0,0 +1,61 @@
'use strict';

const { getImportIdentifier } = require('../utils/import');
const { isClassPropertyOrPropertyDefinitionWithDecorator } = require('../utils/decorators');

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.';

function visitClassPropertyOrPropertyDefinition(node, context, trackedImportName) {
const hasTrackedDecorator =
node.decorators?.length > 0 &&
isClassPropertyOrPropertyDefinitionWithDecorator(node, 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' });
}
}

//------------------------------------------------------------------------------
// 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
}
},
ClassProperty(node) {
visitClassPropertyOrPropertyDefinition(node, context, trackedImportName);
},
PropertyDefinition(node) {
visitClassPropertyOrPropertyDefinition(node, context, trackedImportName);
},
};
},
};
99 changes: 99 additions & 0 deletions tests/lib/rules/no-tracked-properties-from-args.js
@@ -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: [
Copy link
Member

Choose a reason for hiding this comment

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

Another valid test case: @tracked test = this.notArgs.args.test

`
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
},
],
},
],
});