Add new rule `no-assignment-of-untracked-properties-used-in-tracking-contexts` #855

bmish · 2020-06-16T18:27:44Z

See rule documentation file for explanation and examples.

CC: @mongoose700
CC: @pzuraq who added this assertion originally

mongoose700 · 2020-06-16T19:42:23Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+
+  if (types.isClassDeclaration(nodeClass)) {
+    // Native class.
+    nodeClass.body.body.forEach((node) => {


nit: I would prefer a flatMap here to forEach and push(...).

mongoose700 · 2020-06-16T19:45:01Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+function findTrackedProperties(nodeClassDeclaration) {
+  const results = [];
+
+  nodeClassDeclaration.body.body.forEach((node) => {


nit: This could use filters and a map.

mongoose700 · 2020-06-16T19:46:55Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+    }
+
+    // State being tracked for this file.
+    const trackedProperties = new Set();


If someone declared multiple classes in the same file, this wouldn't get reset between them.

Fixed as per other comment, and added a test case for this.

mongoose700 · 2020-06-16T19:48:26Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+          currentEmberModule = node;
+
+          // Gather computed property dependent keys from this class.
+          findComputedPropertyDependentKeys(node).forEach((key) =>


nit: I think it would be better to do

let computedPropertyDependentKeys = new Set(findComputedPropertyDependentKeys(node))

instead of using forEach and add. It would help with making sure we clean up state from any previous classes.

Much better, fixed.

mongoose700 · 2020-06-16T19:52:20Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+        while (current.object.type === 'MemberExpression') {
+          current = current.object;
+        }
+        if (current.object.type !== 'ThisExpression') {


Technically we could be in a nested function that wouldn't actually be called with the class object for this. Though if we tried to account for all cases like that, we wouldn't be able to make a fix for this rule at all, because you could call any method with a different this if you really wanted to.

Good point, I'm going to ignore that for now.

mongoose700 · 2020-06-16T19:54:20Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+          node,
+          message: ERROR_MESSAGE,
+          fix(fixer) {
+            return fixer.replaceText(node, `this.set('${propertyName}', ${nodeTextRight})`);


If making it a tracked property is an option, should we do that instead?

That's a much bigger change, and would be much more likely to cause behavior changes. Might be better to save that for either an optional rule option or codemod.

https://github.com/ember-codemods/ember-tracked-properties-codemod

mongoose700 · 2020-06-16T19:54:55Z

lib/utils/ember.js

@@ -223,6 +224,19 @@ function isEmberCoreModule(context, node, moduleName) {
  return false;
 }

+function isAnyEmberModule(context, node) {


Should this include Helper?

It probably should so I have added helper to this list.

mongoose700 · 2020-06-16T19:56:32Z

tests/lib/rules/no-assignment-of-computed-property-dependencies.js

+      import Component from '@ember/component';
+      class MyClass extends Component {
+        @computed('args.x') get prop() {}
+        myFunction() { this.args.x = 123; }


This looks like something you shouldn't be doing anyway :p

mongoose700 · 2020-06-18T03:39:48Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+      },
+
+      CallExpression(node) {
+        if (emberUtils.isAnyEmberModule(context, node)) {


The tracked properties need to be reset here.

mongoose700 · 2020-06-18T03:40:59Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+        const nodeTextRight = sourceCode.getText(node.right);
+        const propertyName = nodeTextLeft.replace('this.', '');
+
+        if (propertyName.startsWith('args.')) {


We should make sure that we're in a Glimmer component for this check. You could have an args property that needs to be tracked on a class Ember component, or in a service.

mongoose700 · 2020-06-18T03:46:17Z

tests/lib/utils/ember-test.js

@@ -635,6 +635,29 @@ describe('isEmberObjectProxy', () => {
  });
 });

+describe('isEmberHelper', () => {
+  describe("should check if it's an Ember helper", () => {
+    it('should detect when using native classes', () => {


Could you also test Helper.extends?

lib/rules/no-assignment-of-computed-property-dependencies.js

rwjblue · 2020-06-18T13:34:34Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+  if (types.isClassDeclaration(nodeClass)) {
+    // Native JS class.
+    return javascriptUtils.flatMap(nodeClass.body.body, (node) => {
+      const computedDecorator = decoratorUtils.findDecorator(node, 'computed');


This situation is exactly the same as with observer dependent keys, we should probably account for that in the same rule here.

Also, we really ought to make this check for the imported computed (vs just any method that happens to be named computed).

Updated to use imported name for computed and added a test.

lib/rules/no-assignment-of-computed-property-dependencies.js

rwjblue · 2020-06-18T13:40:44Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+
+    return {
+      // Native JS class:
+      ClassDeclaration(node) {


We should probably handle enter/exit here, and push/pop onto a stack of known tracked properties.

For example, a file with the following contents should still warn:

export default class extends Service { @computed('foo') whatever() { // ...snip... } otherFunc() { class InternalStateTracker { @tracked foo; } this.foo = 'lolol'; } }

This is tricky...

Ya, indeed. I was thinking that the stack thing would make it work.

@rwjblue good news, I switched to a stack to handle nested classes, and added a test case for this. Thank you for the inspiration!

rwjblue · 2020-06-18T13:41:56Z

lib/rules/no-assignment-of-computed-property-dependencies.js

+    // State being tracked for this file.
+    let trackedProperties = undefined;
+    let computedPropertyDependentKeys = undefined;
+    let currentEmberModule = null;


This variable name is odd, what does it represent?

The node for the current Ember module (component, controller, etc) that we're inside. Made some slight updates to naming/comments for clarity.

bmish · 2020-06-18T14:02:21Z

@rwjblue I'm planning to rename this rule to no-assignment-of-untracked-properties-used-in-tracking-contexts to be more accurate and to allow it to be expanded to handle other tracking contexts beyond just computed properties (such as observers) in the future. Let me know if you have any thoughts.

rwjblue · 2020-06-18T14:09:20Z

Ya, that sounds good to me. In theory, this could apply to any {{this.foo}} in a template too (and I honestly think we could make that work).

mongoose700 · 2020-06-18T16:47:08Z

lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js

+ * @param {Node} nodeClass - Node for the Ember class
+ * @returns {String[]} - list of dependent keys used inside the class
+ */
+function findComputedPropertyDependentKeys(nodeClass, computedImportName) {


Should we also find things like computed.readOnly or @readOnly? It could also be useful to allow for configuring extra methods to be recognized as creating computed properties, in case people make custom ones.

Good point, I will think about how to support these computed property macros.

mongoose700 · 2020-06-18T16:48:18Z

lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js

+
+  return node.arguments
+    .filter((arg) => arg.type === 'Literal' && typeof arg.value === 'string')
+    .map((node) => node.value);


Do we need to do anything special for @each here?

Good point, updated to handle that, and added a test case.

mongoose700 · 2020-06-18T16:50:38Z

lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js

+
+  return node.arguments
+    .filter((arg) => arg.type === 'Literal' && typeof arg.value === 'string')
+    .map((node) => node.value);


Does this currently handle @computed('foo.bar') and this.foo = 'a' (as opposed to this.foo.bar = 'a')?

Good point, updated to handle that, and added a test case.

mongoose700 · 2020-06-22T17:50:10Z

lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js

+  push(item) {
+    this.stack.push(item);
+  }
+  peak() {


This should be peek instead.

Doh! Fixed.

mongoose700 · 2020-06-22T17:52:07Z

lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js

+          node,
+          computedPropertyDependentKeys,
+          trackedProperties,
+          isInGlimmerComponent,


nit: I think isGlimmerComponent would be better than isInGlimmerComponent in this context.

Agreed. I had made this same change locally but hadn't pushed yet.

mongoose700 · 2020-06-22T17:55:29Z

lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js

+          return;
+        }
+
+        const currentClass = classStack.peak();


nit: You could do

const { isInGlimmerComponent, computedPropertyDependentKeys, trackedProperties } =

mongoose700 · 2020-06-22T17:59:10Z

lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js

+          node,
+          message: ERROR_MESSAGE,
+          fix(fixer) {
+            return fixer.replaceText(node, `this.set('${propertyName}', ${nodeTextRight})`);


Won't this.set be invalid for Glimmer components or non-classic usages of Ember components?

Regarding Glimmer components, I just updated to remove the autofixer, and added a TODO comment saying that we should autofix to use set() once we are capable of adding the import statement for it.

Regarding native components, I will keep the this.set() autofixer since that still works, even though it is deprecated (and can be easily fixed just by switching to the imported version).

Do we also need to worry about Ember components using extends Component { that don't have the classic decorator?

I redid the autofixer to always use the imported set function and add an import statement if needed. So this shouldn't be an issue anymore.

mongoose700 · 2020-06-22T17:59:57Z

lib/utils/ember.js

+    isEmberRoute(context, node) ||
+    isEmberService(context, node) ||
+    isEmberArrayProxy(context, node) ||
+    isEmberObjectProxy(context, node) ||


Should EmberObject be added to this list as well?

I was thinking about that. I have now added it.

mongoose700 · 2020-06-22T20:53:00Z

lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js

+          fix(fixer) {
+            if (currentClass.isGlimmerComponent) {
+              // Glimmer components should have no autofix since `this.set` is not available in them.
+              // TODO: autofix to `set()` once we can add the import: `import { set } from '@ember/object';`


What's the reason we can't do this?

Good news, I went ahead and updated the autofixer to always use the imported set function, and add an import statement if needed.

…cking-contexts`

bmish · 2020-06-23T15:20:36Z

Expecting to merge this later today. Thanks for the great reviews.

bmish added the enhancement label Jun 16, 2020

bmish requested a review from rwjblue June 16, 2020 18:27

mongoose700 reviewed Jun 17, 2020

View reviewed changes

mongoose700 reviewed Jun 18, 2020

View reviewed changes

rwjblue reviewed Jun 18, 2020

View reviewed changes

bmish changed the title ~~Add new rule no-assignment-of-computed-property-dependencies~~ Add new rule no-assignment-of-untracked-properties-used-in-tracking-contexts Jun 18, 2020

mongoose700 reviewed Jun 18, 2020

View reviewed changes

bmish requested a review from rwjblue June 22, 2020 16:26

mongoose700 reviewed Jun 22, 2020

View reviewed changes

feat: add new rule `no-assignment-of-untracked-properties-used-in-tra…

af3e782

…cking-contexts`

mongoose700 approved these changes Jun 23, 2020

View reviewed changes

bmish merged commit 82acc5b into ember-cli:master Jun 25, 2020

Add new rule no-assignment-of-untracked-properties-used-in-tracking-contexts #855

Add new rule no-assignment-of-untracked-properties-used-in-tracking-contexts #855

Conversation

bmish commented Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmish Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmish commented Jun 18, 2020

rwjblue commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmish Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmish Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

mongoose700 Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmish commented Jun 23, 2020

Add new rule `no-assignment-of-untracked-properties-used-in-tracking-contexts` #855

Add new rule `no-assignment-of-untracked-properties-used-in-tracking-contexts` #855

bmish commented Jun 16, 2020 •

edited

Loading

bmish Jun 18, 2020 •

edited

Loading

bmish Jun 22, 2020 •

edited

Loading

bmish Jun 22, 2020 •

edited

Loading

mongoose700 Jun 22, 2020 •

edited

Loading