`assertTagNotConsumed` triggering in class using setters in constructor, when constructor called in Octane autotrack getter

[cwsault]

I have a class which uses getters & setters to perform logic around a @tracked backing property, _backingField. These are accessed in the class constructor.

If an instance of this class is constructed within a native getter which is using autotracking, I receieve an error:

You attempted to update _backingField on ClassName, but it had already been used previously in the same computation. Attempting to update a value after using it in a computation can cause logical errors, infinite revalidation bugs, and performance issues, and is not supported.

It appears that accessing the _backingField within the constructor before later assigning to it in the setters causes this, triggered by assertTagNotConsumed, but only if accessed from an Octane auto-tracking getter. I can build an instance of the class in non-computation code without any errors.

More in-depth example:

I have a class which can, under certain conditions, have a list of country codes. If the condition that enables country codes is true, then the country list must contain 'USA', as well as unique-ify the list. If turned off, then the list of country codes is emptied. The backing fields & setters are used to validate this logic. I've simplified it for this example:

app/objects/my-buggy-class.ts

import { tracked } from '@glimmer/tracking';

export default class MyBuggyClass {
    @tracked _allowsCountries!: boolean;
    @tracked _countries: string[] = [];

    constructor(allowsCountries: boolean, countries: string[] = []) {
        this.allowsCountries = allowsCountries;
        this.countries = countries;
    }

    get allowsCountries(): boolean {
        return this._allowsCountries;
    }

    set allowsCountries(allowsCountries: boolean) {
        this._allowsCountries = allowsCountries;

        if (!allowsCountries) {
            this.countries = [];
        } else if (!this.countries.includes('USA')) {
            this.addCountry('USA');
        }
    }

    get countries(): string[] {
        return this._countries;
    }

    set countries(countries: string[]) {
        if (!this.allowsCountries) {
            countries = [];
        } else {
            countries = ['USA'].concat(countries);
            countries = [...new Set(countries)];
        }
        this._countries = countries;
    }

    addCountry(country: string): void {
        this.countries = this.countries.concat(country);
    }

    get joinedCountries(): string {
        return this.countries.length ? this.countries.join(',') : 'EMPTY';
    }
}

app/components/my-buggy-component.ts

import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import { action } from '@ember/object';
import MyBuggyClass from '@org-name/app-name/objects/my-buggy-class';

export default class MyBuggyComponent extends Component {
    buggyClass = new MyBuggyClass(true, ['USA', 'FRA']);
    @tracked displayCopy = false;

    @action
    toggleDisplayCopy(): void {
        this.displayCopy = !this.displayCopy;
    }

    get copyOfClass(): MyBuggyClass {
        return new MyBuggyClass(this.buggyClass.allowsCountries, this.buggyClass.countries);
    }
}

app/templates/components/my-buggy-component.hbs

<div>
    buggyClass: {{this.buggyClass.joinedCountries}}
</div>

<div>
    <button {{on 'click' this.toggleDisplayCopy}}>Toggle</button>
</div>

{{#if displayCopy}}
    <div>
        copyOfClass: {{this.copyOfClass.joinedCountries}}
    </div>
{{/if}}

In this, when I click the button in MyBuggyComponent, it activates the getter for copyOfClass, then throws an error in the constructor of MyBuggyClass:

Error: Assertion Failed: You attempted to update `_countries` on `MyBuggyClass`, but it had already been used previously in the same computation.  Attempting to update a value after using it in a computation can cause logical errors, infinite revalidation bugs, and performance issues, and is not supported.

`_countries` was first used:

- While rendering:

  application
    my-buggy-component
      this.copyOfClass

Stack trace for the update:
    Ember 2
    assertTagNotConsumed validator.js:216
    setter validator.js:823
    set Ember
    set countries my-buggy-class.js:55
    addCountry my-buggy-class.js:59
    set allowsCountries my-buggy-class.js:39
    MyBuggyClass my-buggy-class.js:25
    get copyOfClass my-buggy-component.js:30
    Ember 3
    combined reference.js:304
    runInAutotrackingTransaction validator.js:121

I'm aware that in this example the button's action could just copy the object, but in the actual use case it's not as straightforward -- there are multiple domain objects holding an instances of the class, and 'parent' domain objects must build a combined version of the lower ones whenever the fields like allowsCountries or countries.[] are altered.

Pre-Octane-upgrade, rather than having a getter for copyOfClass, we would have an observer on the fields of buggyClass and assign the new value of copyOfClass directly. The observers no longer fired with the conversion to autotrack getters in the class though, so I am attempting to transform it into a computation fully derived from the copy-target instances, only to encounter this issue.

Not sure if this is a bug, if I should be changing the class constructor to operate differently (i.e. breaking out the setter logic perhaps & only assigning directly to the tracked backing properties in it, rather than going through the getters & setters), or if I'm not understanding something about how the newer tracking etc. is supposed to work.

[pzuraq]

So this assertion is correct, this is a series of operations that we do not allow. Specifically, you cannot read a tracked property, and then write to it, during a tracked computation, and rendering is a tracked computation.

The reason for this is because it could cause inconsistent output. For instance, something could have read the _countries array a while ago, and rendered it to the DOM. Then, if you changed _countries, we would either have inconsistent output because the list has already been rendered somewhere with different countries, or we would have to rerender. Relying on this type of rerendering can cause infinite loops in the worst case, and performance issues in the best case, so we assert against this in general.

In this specific case, I think we can simplify the logic a bit here. Here's one option that uses tracked-built-ins to create a tracked Set, and manage that state internally. All of the state is demand driven, and doesn't use setters, so it's a bit easier to reason about where state is being set/updated.

import { tracked } from 'tracked-built-ins';

export default class MyBuggyClass {
  @tracked allowsCountries!: boolean;

  _countries = tracked(new Set<string>(['USA']));

  constructor(allowsCountries: boolean, countries: string[] = []) {
    this.allowsCountries = allowsCountries;

    for (let country of countries) {
      this._countries.add(country);
    }
  }

  get countries(): string[] {
    if (this.allowsCountries) {
      return [...this._countries];
    } else {
      return [];
    }
  }

  addCountry(country: string): void {
    this.countries = this._countries.add(country);
  }

  get joinedCountries(): string {
    return this.countries.length ? this.countries.join(',') : 'EMPTY';
  }
}

Now our logic is one directional, and we aren't checking state and then setting it. If your concerned about performance, you could consider using the @cached decorator on the countries getter, but I would measure that first to be sure it's necessary.

[rwjblue]

Going to go ahead and close this, I think @pzuraq covered everything.

[cwsault]

FWIW the way we ended up approaching this was by making the class immutable.

Didn't desire having the countries array & similar fields getting stale internally like they would if the logic for the empty array was done through the getter. & there were additional functions like .removeCountry() that didn't parse well if not using a set, & some other concerns with how our component to mutate this worked with some other logic.

So ended up making it so the fields are immutable & methods like .addCountry() return a new instance with the altered fields, added one for replacing them wholesale instead of the setter, etc.

Immutability works a like nicer with the other components that use this, as we have domain objects that were observing changes in the fields in their children (which happened when our component mutated the instance passed into it) & updating their own class instance. Now instead the builder component we have for this class just takes in an action & calls it with the new instance it creates when e.g. countries are altered. So then the domain objects holding this example one can just do that parent-flow logic in the setter that replaces the entire instance, instead of observing the fields