eslint-plugin-ember icon indicating copy to clipboard operation
eslint-plugin-ember copied to clipboard

`no-assignment-of-untracked-properties-used-in-tracking-contexts` Breaks Code

Open anlumo opened this issue 4 years ago • 4 comments

Consider this code:

import EmberObject, { computed } from '@ember/object';

class Foo extends EmberObject {
    _bar = null;
    @computed()
    get bar() {
        if (this._bar) {
            return this._bar * 2;
        }
        this._bar = 1;
        return 2;
    }
    set bar(value) {
        this._bar = value / 2;
    }
}

The rule no-assignment-of-untracked-properties-used-in-tracking-contexts will transform this into:

import EmberObject, { computed, set } from '@ember/object';

class Foo extends EmberObject {
    _bar = null;
    @computed('_bar')
    get bar() {
        if (this._bar) {
            return this._bar * 2;
        }
        set(this, '_bar', 1);
        return 2;
    }
    set bar(value) {
        set(this, '_bar', value / 2);
    }
}

which immediately causes an infinite loop if anything observes bar and you try to set the value, because the getter causes the observers on _bar to fire, which causes the observers on bar to fire, etc.

anlumo avatar Dec 06 '21 23:12 anlumo

It looks like you're seeing the autofix results of two rules:

  1. ember/no-assignment-of-untracked-properties-used-in-tracking-contexts changing assignment to set (note: there's a ticket about this becoming a suggestion and not an autofix: https://github.com/ember-cli/eslint-plugin-ember/issues/977)
  2. ember/require-computed-property-dependencies adding a missing dependency

Independently, the autofix from each of these rules looks correct to me. But when combined together, they cause a problem.

While it's possible these lint rules could be tweaked to avoid this edge case, the most glaring issue I'm seeing here is the way the code sample is written. It's generally considered bad practice for getters to have side effects (like modifying other properties). Side effects like this can cause all sorts of weird and unexpected behavior. And the lint rule ember/no-side-effects is supposed to prevent this situation in the first place.

bmish avatar Dec 07 '21 03:12 bmish

It's generally considered bad practice for getters to have side effects (like modifying other properties).

Well, the code I wrote above is just a minimal example. The reason why I have code like that is that the underlying data are WebGL objects, which have two properties:

  • They're very expensive to create, so I have to reuse an existing one instead of creating a new one every time the getter is executed.
  • They don't clean up themselves, I have to call things like deleteTexture or deleteBuffer on them when I don't need them any more, otherwise I get memory leaks. This means that I have to be able to reference them in destroy without calling the getter.

anlumo avatar Dec 07 '21 15:12 anlumo

Definitely makes sense what you're trying to do, but still I don't think the code sample is written correctly. I have seen people try to write code like that before and almost always there's a cleaner way to re-architect the logic to avoid side effects in getters. And if the computed property is written correctly, the built-in caching provided by computed properties will help address your performance concerns and prevent the code from being executed multiple times.

bmish avatar Dec 07 '21 17:12 bmish

I'm not aware of ember providing any solution for this, but then again I don't know all of the internal code and the documentation about property caching is rather minimal. I'm using ember-data's PromiseObject wherever possible, but that has caused even more headaches than my homebrew second layer to the cache system.

However, I don't think that this is the right place for a discussion about how to do this in a different way, so I'm moving this to the Discord server.

anlumo avatar Dec 07 '21 17:12 anlumo