[Bug/Regression] 3.22.0-beta.1 triggers update assertion

[nickschot]

🐞 Describe the Bug

Since 3.22.0-beta.1 I've seen the update assertion triggered in non-production environments (so development serve/build & tests) when using ember-set-body-class.

I've traced it back to https://github.com/ef4/ember-set-body-class/blob/master/addon/services/body-class.js#L14 which is triggered when the set-body-class component is unrendered.

🔬 Minimal Reproduction

https://github.com/nickschot/body-class-reproduction

Press the toggle button twice and the assertion should show up in the console.

😕 Actual Behavior

The following assertion is shown:

index.js:172 Uncaught (in promise) Error: Assertion Failed: You attempted to update `[]` on `Array`, 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.

`[]` was first used:

- While rendering:
  application

🤔 Expected Behavior

To not have the assertion shown. The set-body-class addon is quite old and has worked fine up until 3.21.

🌍 Environment

• Ember: 3.22.0-beta.1
• Node.js/npm: -
• OS: macOS Catalina
• Browser: Chrome

➕ Additional Context

Some related chatter in Discord: https://discordapp.com/channels/480462759797063690/485447409296736276/748256841573859389

An addon of mine which has the assertion in CI: https://travis-ci.org/github/nickschot/ember-mobile-menu/jobs/721443473#L439

I've also deployed a production build to github pages, but as I said before the assertion does not trigger in production so... : https://nickschot.github.io/body-class-reproduction/

[rwjblue]

Hmm, this doesn't really seem like a bug here in Ember's code to me. AFAICT the thing the assertion is telling you is actually a problem: the array of components is consumed / accessed everywhere that {{set-body-class component is used, any removals that happen during rendering will then attempt to mutate that array.

I suspect that prior to the recent refactorings to embrace auto-tracking in the rendering engine there were spots during rendering that were not being auto-tracked (e.g. where willDestroyElement was called).

[nickschot]

Hmm, this doesn't really seem like a bug here in Ember's code to me. AFAICT the thing the assertion is telling you is actually a problem: the array of components is consumed / accessed everywhere that {{set-body-class component is used, any removals that happen during rendering will then attempt to mutate that array.

I suspect that prior to the recent refactorings to embrace auto-tracking in the rendering engine there were spots during rendering that were not being auto-tracked (e.g. where willDestroyElement was called).

Definitely valid and I suspected this was the case! In that case I'll create an issue in the set-body-class repo.

[amk221]

My entire addon has also blown up on this version

I've not looked into it yet, but I kind of hope @nickschot is right :)

[amk221]

export default class MyComponent extends Component {
  @tracked value = 1;
  previousValue = null;

  constructor() {
    super(...arguments);

    this.previousValue = this.value;
    this.value = 2;
  }
}

Error: Assertion Failed: You attempted to update `value` on `MyComponent`, 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.

`value` was first used:

- While rendering:
  application
    my-component

repo

[rwjblue]

Hmm, that snippet you shared there @amk221 is definitely something that should not be allowed (reading from a tracked property and then setting it after its been read). My guess is that this is an attempt to simplify your real world scenario down?

[amk221]

Yeah, that's what my addon is doing :/ (I need to keep track of the previous value. I'm struggling to build a mental model of why this isn't allowed).

[rwjblue]

@amk221 - Sure, but on initialization (when the value is also not coming from an argument) there is no need to initialize and update back to back.

[amk221]

I see... I suppose the reproduction does look silly. In the addon code, the set is called like this

constructor() {
  super(...arguments)
  this.receiveValue()
}

@action
handleUpdate() {
  this.receiveValue()
}

<div {{did-update this.receiveValue @value}}></div>

So to fix it, I would have to un-dry the code and make the constructor function behave differently?

Anyway, apologies for hijacking.. I will have to crack on and fix it then

[rwjblue]

Ya, so basically the thing that is (and should) throw an error is reading from a tracked property then updating the tracked property in the same render cycle.

AFAICT, that is exactly what this code is doing:

https://github.com/zestia/ember-select-box/blob/74d833a93c9b9f4915c996c7e39533a7818c391f/addon/utils/component/value.js#L21-L27

Which is actually quite close to the reduced reproduction snippet you pasted above:

export default class MyComponent extends Component {
  @tracked value = 1;
  previousValue = null;

  constructor() {
    super(...arguments);

    this.previousValue = this.value;
    this.value = 2;
  }
}

[amk221]

Thanks for taking the time.

[rwjblue]

Ya, no problem, sorry for the troubles though.

[rwjblue]

@amk221 - FWIW, I created https://github.com/ember-learn/guides-source/issues/1528 to track getting some better documentation in the auto-tracking in depth guide that would help explain why the assertion exists (and what it protects you from).

[pzuraq]

@amk221 I think the key issue in your flow is when the previous value is being set. In the initial state, value is null, so there's no reason to read it and set the previous value to it. You can instead set previousValue immediately after firing the action, in this function: https://github.com/zestia/ember-select-box/blob/c61e38129cb3f3eefbb82bf0a6125b9f8b2d2f71/addon/utils/shared/value.js#L28. This is when the information of what is "previous" becomes relevant.

I also think it's somewhat of an antipattern to trigger actions based on rendering. At the least, it could cause multiple render passes, or cause you to run into more of the backtracking rerender assertion. Maybe it would make sense to have a different flow specifically for initialization, where the check against the previous value (and calling the action) are skipped.

I'm planning on writing a blog post to dig a bit more deeply into why the assertion exists in the first place. It definitely requires some getting used to in terms of the constraints, but it can be really helpful and catch some really tricky bugs!

[amk221]

@pzuraq Thank you! (Can you elaborate on “it's somewhat of an antipattern to trigger actions based on rendering”? Is this referring to some specific code your saw in the add on?)