emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.46k stars 4.21k forks source link

[Bug] Regression in 5.6.0 concerning auto-tracking #20657

Open boris-petrov opened 3 months ago

boris-petrov commented 3 months ago

🐞 Describe the Bug

A couple of my tests started blowing up with the update-after-read assertion after upgrading from 5.5.0 to 5.6.0. Still happens in 5.7.0.

🔬 Minimal Reproduction

Repo

Clone, pnpm install, ember serve, open the page, look at it blow up (in the console) one second later. Downgrading to 5.5.0 fixes the problem. Also, funnily, adding {{#if this.results}}{{/if}} (or anything relating this.results) in parent.hbs also makes it stop blowing up.

😕 Actual Behavior

Autotracking assertion.

🤔 Expected Behavior

No assertion.

🌍 Environment

cc @NullVoxPopuli

P.S. Note this seems to happen because of the style modifier. If it is removed, the code works fine even though it's "touching" the same things. Not sure why...

NullVoxPopuli commented 3 months ago

note that the repro is called ember-resources-bug but ember-resources isn't used here :sweat_smile:

it's using ember-could-get-used-to-this, which uses a side-effecting callbacks on a class as part of the Helper lifecycle.

@boris-petrov and I talked about this on Discord a good bit, and I think that the lack of error was actually a bug, but what's goofy with this repro is that "the work-around" needs an explanation -- which I haven't looked in to why ends up working out that way -- but seems suspicious.

This is the resource in the repro:

import { tracked } from '@glimmer/tracking';
import { Resource } from 'ember-could-get-used-to-this';

export default class LiveSearchResource extends Resource {
  @tracked results;

  get value() {
    return this.results;
  }

  setup() {
    this.update();
  }

  update() {
    this.results = [this.args.named.counter];
  }
}

which, I would expect that setting tracked data synchronously during create/update would cause a back-tracking assertion, as create and update in HelperManager (capabilities 3.23+) have auto-tracked create/update calls.

The Todos:

boris-petrov commented 3 months ago

Ah, sorry for the wrong name! Yes, the "resources" in the name is the generic Ember concept, not the specific implementation of @NullVoxPopuli. :)

We did discuss at length, yes. However, I'm still not convinced that @NullVoxPopuli is right in that particular case. As I mentioned in the conversation with him:

I think the resource is created, setup is called and only then get value is called. Same for update - it is called first and then get value.

Which is a read-after-write which should not trigger the assertion.

Also, there is the fact that removing the style modifier call - the code works fine. Even though the same things happen - the same values get "touched" at the same times. Just no modifier call.

Thirdly, there is this strangeness with {{#if this.results}}{{/if}}.

I might be wrong about all this, of course. You're the Ember gurus. :)