Backtracking assertion "more aggressive" in 3.22

[Yelinz]

🐞 Describe the Bug

The autotracking.mutation-after-consumption deprecation throws an error which breaks apps instead of a warning

🔬 Minimal Reproduction

https://ember-twiddle.com/c677ed60d39938c97ea7438d177c0ef3?openFiles=controllers.application%5C.js%2C

The console has to be opened to see the error

😕 Actual Behavior

Deprecation throws an error

🤔 Expected Behavior

Deprecation is a warning

🌍 Environment

• Ember: 3.22.0
• Node.js/npm: -
• OS: -
• Browser: -

[rwjblue]

Hmm, I don't fully understand what you mean here. The error that is thrown is an assertion (not a deprecation) and is not intended to be able to be worked around...

[rwjblue]

I'm going to close for now, but I'm happy to reopen if I've misunderstood something here...

[Yelinz]

ember.js/packages/@ember/-internals/glimmer/lib/environment.ts

Line 65 in c5bbf86

id: 'autotracking.mutation-after-consumption',

This is the supposed deprecation which should run, but why is the assertion causing an error instead of putting up a warning. The error causes apps to break instead of showing a warning to refactor it

[rwjblue]

The spot you linked to is where Ember configures the rendering engine, it provides hooks for both assertions and deprecations and the rendering engine picks which one to use based on the scenario.

Those configured options turn into running either of the following in glimmer-vm:

• runInTrackingTransaction

• deprecateMutationsInTrackingTransaction

---

Can you describe more why you think this specific case should be a deprecation instead of an assertion? The specific thing being done in the twiddle was always intended to be prevented (as the error message explains):

export default class ApplicationController extends Controller {
  @tracked appName = 'Ember Twiddle';

  get name() {
    this.appName = "Test"
    
    if (this.appName === "Test") {
      this.appName = "test"
    }
    
    return this.appName
  }
}

Specifically, in this case you are reading a tracked value then updating it (which is effectively going to cause a cycle if we didn't prevent it with this assertion).

[kpfefferle]

@rwjblue I think the issue here is that a minor release of Ember is causing apps that were previously unknowingly violating this assertion to now be blocked from upgrading until their backtracking updates are resolved. In my case, an app I maintain is blocked from upgrading to 3.22 because an addon it consumes violates this assertion.

If these backtracking assertions were introduced as warnings instead of errors, it would give apps like mine an easier path to staying up-to-date since we'd be given some time to track down these issues. As it currently stands, my app has to stay on 3.21 until I have the bandwidth to track down where/why it's violating this new assertion.

[Yelinz]

Why I think it is a bug is because ember-gestures has stopped working with Ember 3.22
They are using this function on tear down and it worked with the previous versions.
The variable __instance is being read and then updated, so that means it doesn't work any more and now causes apps to break, even though it wasn't a major version change

  __teardownRecognizers: on('willDestroyElement', function() {
    let instance = this.get('__instance');
    if (instance) {
      //instance.off();
      instance.destroy();
      this.set('__instance', null);
    }
  }),

[rwjblue]

I think the issue here is that a minor release of Ember is causing apps that were previously unknowingly violating this assertion to now be blocked from upgrading until their backtracking updates are resolved. In my case, an app I maintain is blocked from upgrading to 3.22 because an addon it consumes violates this assertion.

This is great! The original report did not indicate that the code in the twiddle ever actually worked (or any reasoning about why it was an error).

[rwjblue]

@kpfefferle - From what version are you upgrading? What is the source of mutation that is causing the assertion in your case?

[rwjblue]

To be clear, I'm mostly trying to understand what is being reported here (scope, usage patterns, etc), and I'm specifically not attempting to rationalize or argue about SemVer. 😸

[kpfefferle]

@rwjblue I have two apps with failing Dependabot PRs to bump them from 3.21.3 to 3.22.0. Both show similar error messages for all failing tests, something along the lines of:

    ---
        actual: >
            [object Object]
        stack: >
            Error: Assertion Failed: You attempted to update `_errors` on `changeset:[object Object]`, 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.
            
            `_errors` was first used:
            
            - While rendering:
              application
                forms
                  forms.form
                    form-workflow
                      form-workflow/form
                        form-for
            
            Stack trace for the update:
                at setter ([...]/assets/vendor.js:56179:9)
                at EmberChangeset.set [as _errors] ([...]/assets/vendor.js:17775:7)
                at EmberChangeset.BufferedChangeset.set (webpack://__ember_auto_import__/./node_modules/validated-changeset/dist/validated-changeset.es5.js?:2674:17)
                at Object.set ([...]/assets/vendor.js:104899:22)
                at Proxy.BufferedChangeset._handleValidation (webpack://__ember_auto_import__/./node_modules/validated-changeset/dist/validated-changeset.es5.js?:2447:18)
                at Proxy.BufferedChangeset._validateKey (webpack://__ember_auto_import__/./node_modules/validated-changeset/dist/validated-changeset.es5.js?:2425:23)
                at eval (webpack://__ember_auto_import__/./node_modules/validated-changeset/dist/validated-changeset.es5.js?:2211:24)
        message: >
            Assertion Failed: You attempted to update `_errors` on `changeset:[object Object]`, 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.
            
            `_errors` was first used:
            
            - While rendering:
              application
                forms
                  forms.form
                    form-workflow
                      form-workflow/form
                        form-for

This seems to be the internals of ember-changeset-validations doing some recomputation of errors, but I haven't had the bandwidth to dig into it yet. While the assertion seems to be accurate and valid, having it blow up my test suite means I'm blocked from upgrading to 3.22 until I can figure out what's going on.

[rwjblue]

Gotcha. FWIW the error message does look at least "useful" to help debugging.

@Yelinz - I don't think the original issue reported (in the twiddle) is in the same realm here (that twiddle errors since at least 3.16) as what @kpfefferle is reporting. Is this twiddle representative of your actual situation? What version are you upgrading from?

[Yelinz]

Like I said in my previous comment its from 3.21 to 3.22 and the situation is similar to @kpfefferle just with a different addon. The twiddle I made was just to evoke that error. The actual code which is causing it is the one I posted earlier.

2020-10-12T09:38:40.7310067Z         actual: >
2020-10-12T09:38:40.7310575Z             [object Object]
2020-10-12T09:38:40.7311080Z         stack: >
2020-10-12T09:38:40.7314024Z             Error: Assertion Failed: You attempted to update `__instance` on `<ember-caluma@component:x-toggle-switch::ember726>`, 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.
2020-10-12T09:38:40.7315638Z             
2020-10-12T09:38:40.7316183Z             `__instance` was first used:
2020-10-12T09:38:40.7316698Z             
2020-10-12T09:38:40.7317709Z             - While rendering:
2020-10-12T09:38:40.7318285Z               application
2020-10-12T09:38:40.7318787Z                 demo
2020-10-12T09:38:40.7319293Z                   application
2020-10-12T09:38:40.7319772Z             
2020-10-12T09:38:40.7320295Z             Stack trace for the update:
2020-10-12T09:38:40.7321118Z                 at dirtyTagFor (http://localhost:7357/assets/vendor.js:74216:9)
2020-10-12T09:38:40.7322185Z                 at markObjectAsDirty (http://localhost:7357/assets/vendor.js:31365:32)
2020-10-12T09:38:40.7323337Z                 at notifyPropertyChange (http://localhost:7357/assets/vendor.js:31403:5)
2020-10-12T09:38:40.7324352Z                 at _set2 (http://localhost:7357/assets/vendor.js:32379:9)
2020-10-12T09:38:40.7325263Z                 at Class.set (http://localhost:7357/assets/vendor.js:45923:29)
2020-10-12T09:38:40.7326501Z                 at Class.__teardownRecognizers (http://localhost:7357/assets/vendor.js:165238:14)
2020-10-12T09:38:40.7327580Z                 at sendEvent (http://localhost:7357/assets/vendor.js:30998:14)

The stack trace is also more or less similar to his

[rwjblue]

The twiddle I made was just to evoke that error

Ya, but there are lots of scenarios where that error is supposed to be thrown (and has been thrown since tracked was introduced). The thing we need to figure out is what new scenario is now being tracked that previously was not being tracked. I need to understand the usage pattern in order to do that.

Once we find out what the usage pattern was we can figure out if we can change things to emit a deprecation instead of an assertion, but at the moment we don't yet have a reproduction that represents the real world issues y'all are hitting so I can't really tell if we can modulate the assertion into a deprecation...

[rwjblue]

For any others coming along here, the thing that I'd like to see here is a reproduction (ideally in the form of test failures on some repo; whether an addon or just a demo app that was made for this purpose) that passes on 3.21 and fails on 3.22.

[Yelinz]

This is our passing test on ember 3.21
https://github.com/projectcaluma/ember-caluma/pull/1065/checks?check_run_id=1241360614

Meanwhile the same code fails on 3.22
https://github.com/projectcaluma/ember-caluma/pull/1065/checks?check_run_id=1241408429

The issue comes from a long list of dependencies which ends in ember-gestures and the code snippet I posted earlier

[rwjblue]

Thanks for all of your help here folks (and for bearing with me as I tried to figure out what was going on)!

I'll be working to get this out in patch releases shortly.

[kpfefferle]

@rwjblue Thanks to you and @pzuraq for tracking down the issue!

[GavinJoyce]

👋 @rwjblue, are you still hoping to release a 3.22.1 patch with this fix? I'm keen to upgrade our app to 3.22, please let me know if there is anything that I can do to help.

[john-griffin]

We have encountered this in a bunch of our apps due to a pattern established around ember-changeset-validations. Here is an example of the pattern we use:

import Component from '@glimmer/component';
import Changeset from 'ember-changeset';
import lookupValidator from 'ember-changeset-validations';
import OrderValidations from '../validations/order';

export default class OrderFormComponent extends Component {
  constructor() {
    super(...arguments);

    let model = this.args.model;
    this.changeset = new Changeset(
      model,
      lookupValidator(OrderValidations),
      OrderValidations
    );
    this.changeset.validate();
  }
}

this.changeset.validate() triggers the assertion failure in 3.22.0.

[rwjblue]

I just triggered the release of v3.22.1 (https://github.com/emberjs/ember.js/releases/tag/v3.22.1), should be published to NPM shortly (once CI is completed).

[kpfefferle]

Was this fix actually included in the v3.22.1 release, or maybe did the associated PR not entirely correct this issue? I see #19193 in the release notes, but my project is still showing the same test failures as before 🤔

[pzuraq]

@GavinJoyce is the example that you provided with Glimmer components? We only intended to fix this bug in Classic components for backwards compatibility reasons, in Glimmer components and in general moving forward, we do expect mutations like this to fail during render. In general, it is not possible to safely read tracked state and then write to tracked state during a single tracked computation.

We intended to cover all of these types of mutations since the assertion was originally strengthened in 3.15, but constructors were missed due to the fact that the VM internally was not using autotracking.

We can wrap this with the deprecateMutationsInAutotrackingFrame helper so that it throws a deprecation instead of an assertion for the time being, to prevent this from regressing apps.

[rwjblue]

We can wrap this with the deprecateMutationsInAutotrackingFrame helper so that it throws a deprecation instead of an assertion for the time being, to prevent this from regressing apps.

Ya, this makes sense. That will unblock folks for now, but we do still need applications/addons/etc to fix the underlying issue (we just can't support "read then write" within a tracking context).

[GavinJoyce]

is the example that you provided with Glimmer components?

Yes, it's a Glimmer component.

[GavinJoyce]

Here's an example of a real-world issue that we're seeing that's currently blocking us from upgrading: GavinJoyce/backdrifts#5

Mutating model data (calling addFragment in this case) in the constructor is certainly an anti-pattern, we should very likely do this kind of model mutation when we're creating the model in the route or in the actions phase. Emitting a deprecation instead of an assertion would give apps that encounter issues like this some time to do this refactoring.

[rwjblue]

Emitting a deprecation instead of an assertion would give apps that encounter issues like this some time to do this refactoring.

Absolutely agree! I'm going to try to work on this this morning.

[GavinJoyce]

Thanks, LMK if I can help in any way

[rwjblue]

This will require some changes in glimmer-vm which I submitted in glimmerjs/glimmer-vm#1205, but the Ember integration (to make it a deprecation) is in #19282.

[rwjblue]

Just landed the Glimmer changes needed and I’ll try to get releases out tomorrow.

[rwjblue]

It was a bit more involved than I had originally thought, since we have to ensure that all object types have the deprecation (vs assertion) behaviors. I think #19282 is finally ready for review though.

[rwjblue]

Next steps here are do to backport and test the changes from glimmer-vm in a bunch of different versions (where each of them have breaking changes internally amongst them):

• Backport in glimmer-vm
  • glimmer-vm@0.66 - current master branch targeting ember-source@3.25
  • glimmer-vm@0.65 - current beta branch targeting ember-source@3.24
  • glimmer-vm@0.62 - current release branch for ember-source@3.23
• Backport the changes from Issue deprecations for tracked mutation in constructor during rendering #19282
  • beta branch for ember-source@3.24
  • release branch for ember-source@3.23
• Release changes
  • ember-source@3.23

I'm not 100% sure yet, but it is somewhat likely that I'll release this in 3.23.1+ (since 3.22 is no longer the current release). Does anyone have a specific need for this in 3.22 and can't update to 3.23 instead?

[jherdman]

I'm more than happy to update to 3.23.

Maybe I need another coffee, but I sheepishly admit that I don't fully understand the pattern we're supposed to avoid. I understand the example where you set a value in a getter, but I'm not really sure why Gavin's example is bad (even if it is a bit contrived). Would someone be able to help educate me on the matter?

[GavinJoyce]

I'm not quite sure if there is much more to do to upgrade our app to either 3.22 or 3.23 yet. We have hundreds of failing tests on 3.22, the overwhelming majority of them seem to be backtracking assertions which I'm hopeful will be fixed by your change. Hopefully we can jump straight from 3.21 to 3.23.1.

If it isn't much additional burden, perhaps it would be good to release 3.22.2 too? It might save people who upgrade one version at a time some headache in future.

[jherdman]

FYI I'm seeing this with ember-beta test runs in an Engine:

Offending code:

<LinkToExternal @route="your-coach">{{@coachUser.firstName}}</LinkToExternal>

[rwjblue]

@jherdman make sure you are on the latest ember-engines code, link-to-external was recently updated to avoid this assertion (in ember-engines/ember-engines#735).

[jherdman]

I regret to say I am indeed on v0.8.8 of engines. I don't want to derail this issue, shall I open an issue there?

[rwjblue]

I regret to say I am indeed on v0.8.8 of engines. I don't want to derail this issue, shall I open an issue there?

Yes please!

[rwjblue]

Maybe I need another coffee, but I sheepishly admit that I don't fully understand the pattern we're supposed to avoid. I understand the example where you set a value in a getter, but I'm not really sure why Gavin's example is bad (even if it is a bit contrived). Would someone be able to help educate me on the matter?

The conceptual issue here is that when you mutate a tracked value that has already been read/consumed in the current "tracking frame" we cannot guarantee that the final rendered value is valid. For example, imagine this setup:

• something in your application route accesses {{this.someService.randomTrackedProperty}}
• then later somewhere in a nested route's template you invoke a component like <FooBar />
• in FooBars constructor it does this.someService.randomTrackedProperty = "hello!"

That component mutating a previous read tracked value means that any other references to that value are now invalid.

In Gavins example above he was incrementing a counter, this necessarily means that you are reading the existing value and then updating to a new value. I know it seems innocuous because the value is local to the component and in that example no other code had access to the "invalid" version (the initial version prior to mutating), but in reality there is nothing special about the constructor!. The tracked system is a holistic model of reactivity, and there really isn't anything specialized about component/helper/modifier initialization (the system works just as well if you are not rendering anything at all).

[rwjblue]

Quick update here, 3.23.1 is released with the fixes. I’ll try to finish up 3.22.x tomorrow (ran into issues cherry picking).

[rwjblue]

OK, 3.22.2 is tagged and should be published to NPM shortly.

[GavinJoyce]

Thanks for tagging 3.22.2, I think it will be helpful for us to jump to that first.

FYI, it doesn't seem to be published to NPM 10 hours later: https://www.npmjs.com/package/ember-source/v/3.22.2

[rwjblue]

Just fixed, sorry about that

[GavinJoyce]

Great, thanks!

[tansongyang]

What then is the alternative? Aren't there use cases where you would want to store a previous value before setting to a new value? In such cases, referencing the tracked property before setting it seems unavoidable. For example, something like this:

  this.previousValue = this.trackedProperty;
  this.trackedProperty = newValue;

[pzuraq]

@tansongyang you can store the previous value in untracked state:

this.previousValue = this.currentValue;
this.trackedProperty = this.currentValue = newValue;

[tansongyang]

@pzuraq I see. Thanks for the tip - hadn't thought of that!

We can do that for now, but that does seem a little clunky - are there any plans for this use case to be better, or is it even possible to make it better?

[pzuraq]

There are not plans currently, mainly because this use case is pretty rare, and the downsides of being able to read state without tracking it are pretty large. It can cause you to, quite easily, create situations in which your state is not syncing up properly with your UI and rendered output, for instance.

If we were to introduce such an API, it would likely look something like:

untrack(() => {
  this.previousValue = this.trackedProperty;
});
this.trackedProperty = newValue;

So the ergonomics would probably be pretty similar to the above.

If you would like to see this feature, then I think the best way to help out would be to show some motivating examples. If there are common and important use cases where disabling autotracking is the best way to create an idiomatic solution, then it will help to make the case that we should add the API. So far, we've only see uncommon cases, and the preferred solution is to do something like the example above because it's relatively rare.