[BREAKING BUGFIX beta] Adds autotracking transaction

[pzuraq]

The What

Adds a more strict assertion for autotracking, which prevents users from accidentally using and then mutating a tracked property in the same autotracking frame.

This throws an error in most cases, except when using get and set to mutate inside a class based helper. Since that API has been around for quite some time, it's likely that autotracking being enabled now will cause some issues in the ecosystem if we begin asserting, even though it may make sense to do so.

The Why

Let's look at a small example component:

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

export default class MyComponent extends Component {
  @tracked isLoading = false;
  @tracked models;

  get loadValue() {
    if (!this.isLoading) {
      this.isLoading = true;

      fetch('my.api.com/models').then((models) => {
        this.models = models;
        this.isLoading = false;
      });
    }
  }
}

{{this.loadValue}}

{{#isLoading}}
  <Loading />
{{else}}
  <DisplayModels @models={{this.models}} />
{{/if}}

Today, this would work, and it would have worked in the past if we modeled the same logic using ComputedProperty. However, we are dangerously close to triggering a number of problematic edge cases.

1. We're changing a value just before rendering it (isLoading). If we were to flip the order of the invocations in the template, we would definitely run into issues, since the template would be out of date as soon as we updated the value:

{{#isLoading}}
  <Loading />
{{else}}
  <DisplayModels @models={{this.models}} />
{{/if}}

{{this.loadValue}}

This would trigger the backflow-rerender assertion that has existed in Ember for quite some time now. Without this assertion, it would be very difficult to know for sure if your template would be correct by the end of the render, and you could easily end up in infinite-invalidation cycles.

2. More generally, we're using some state to perform a computation, and then immediately updating that state. In this case, it's not so huge of an issue, but in more complicated getter logic, this could quickly become difficult to reason about. For instance, consider this receipt class:

class Receipt {
  @tracked isSeniorCitizen

  @tracked items = [
    {
      name: 'Bread',
      price: 2
    },
    {
      name: 'Carrots',
      price: 1
    }
  ];

  get total() {
    let sum = this.items.reduce((sum, item) => sum + item.price, 0);

    if (this.isSeniorCitizen) {
      this.items = [
        ...this.items
        {
          name: 'Senior Discount',
          price: -(0.15 * sum)
        }
      ];
    }

    return sum;
  }
}

We can tell that it has a bug, because the sum we calculated in total doesn't take into account the fact that items has changed. This is also a relatively small and isolated occurrence, but in a much larger system this could get very difficult to reason about.

The new assertion, like the previous backtracking-rerender assertion is designed to prevent incorrect usage in this way. Instead, local state and untracked fields/properties can be used when needed for internal state, ensuring that tracked state is always used consistently:

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

export default class MyComponent extends Component {
  @tracked models;

  get isLoading() {
    return !this.models;
  }

  get loadValue() {
    if (!this._isLoading) {
      this._isLoading = true;

      fetch('my.api.com/models').then((models) => {
        this.models = models;
        this._isLoading = false;
      });
    }
  }
}

Notes

• Removes the previous render transaction in favor of a tracking based transaction
• Removes the debugStack in favor of the debugRenderTree
• Adds the env to many of the RootReferences. This is necessary to get the debugRenderTree and log the current render stack when the refs are actually created.
• Helpers and modifiers are also tracked, but they don't currently have access to the debugRenderTree. Helper references need a bit of a refactor, and I wanted to do it after we land the VM upgrade. Modifiers run after render, so I'm actually unsure how to get them the correct logging info.
• Adds a stack trace for both the consumption and mutation locations (first usage and attempted update) which the user can use to jump right to the relevant code.
• I also ran all of ember-observers tests against this change, without the warning logic for helpers, and they were green. The warning helper is really extra precaution, since some of our tests were failing.

[rwjblue]

Thanks for continuing to work on this, sorry for all the nit-picky reviews!

From my perspective this seems like a non-trivial shift and could reasonably be considered a breaking change given that @tracked has already been included in a couple of stable releases (even if it were all warnings, the "console flooding" would still be pretty bad). I just want to ensure that the extra difficulty for users is worth it (e.g. that we can justify the "breakage"), and that the we (framework core team) are all on the same page.

Can you also include information in the PR description explaining the motivation for this change? The current info does a good job of explaining what this PR does, but doesn't really explain why we'd want to do that. I think that would be a decent start to ensuring we can justify things and that we are all on the same page.

[rwjblue]

Left some inline comments, looking much better from my perspective.

Can you include a sample of the expected assertion/deprecation messages in the PR description?

[rwjblue]

Suggested change

	['@test gives helpful assertion when a tracked property is mutated after access in with an autotracking transaction']() {
	['@test gives helpful assertion when a tracked property is mutated after access in an autotracking transaction']() {