Obscure error message when upgrading to 4.7.1

[kpfefferle]

Description

When attempting to upgrade to 4.7.1, we have a couple of tests failing with the following error:

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

The items being referenced don't exist in our app code, so we're not sure what exactly we might be doing to trigger this error. Would you have any ideas what might have changed that we're now infringing on @runspired?

Versions

yarn list v1.22.19
warning Filtering by arguments is deprecated. Please use the pattern option instead.
└─ ember-source@4.7.0
✨  Done in 0.44s.
yarn list v1.22.19
warning Filtering by arguments is deprecated. Please use the pattern option instead.
└─ ember-cli@4.6.0
✨  Done in 0.43s.
yarn list v1.22.19
├─ @ember-data/adapter@4.7.1
├─ @ember-data/canary-features@4.7.1
├─ @ember-data/debug@4.7.1
├─ @ember-data/model@4.7.1
├─ @ember-data/private-build-infra@4.7.1
├─ @ember-data/record-data@4.7.1
├─ @ember-data/rfc395-data@0.0.4
├─ @ember-data/serializer@4.7.1
├─ @ember-data/store@4.7.1
├─ @types/ember-data__adapter@4.0.0
├─ @types/ember-data__model@4.0.0
├─ @types/ember-data__serializer@4.0.0
├─ @types/ember-data__store@4.0.0
├─ @types/ember-data@4.4.1
├─ babel-plugin-ember-data-packages-polyfill@0.1.2
└─ ember-data@4.7.1
✨  Done in 0.43s.

[alexraputa]

@kpfefferle Are you using createRecord in a native getter with the @cached decorator?

@cached
get record() {
  return this.store.createRecord('record');
}

Maybe some code like that caused the error.

[runspired]

I’m seeing this in an app at work and haven’t tracked it down yet. Tag is basically isValid/isError etc flags where ref is the tracked property on it (we create individual companion props for each of these to deal with various requirements of both the computed world and the tracked world)

[runspired]

Incidentally this is why the internals of EmberData don’t use tracked properties, it would pretty much be impossible for us to avoid these sorts of errors if we did 😅

My best guess is that something around property creation/access has started generating a change notification that dirties itself during mutation.

[kpfefferle]

@alexraputa We're not doing that exactly, but the constructor in the "offending" Component does use createRecord to initialize a new data model that is internal to the Component. Something like this:

import Component from '@glimmer/component';
import { service } from '@ember/service';
import StoreService from '@ember-data/store';
import ComponentModel from 'my-app/models/component-model';
import RelatedModel from 'my-app/models/related-model';

interface MyComponentArgs {
  relatedModel: RelatedModel;
}

interface MyComponentSignature {
  Args: MyComponentArgs;
}

export default class MyComponent extends Component<MyComponentSignature> {
  @service declare store: StoreService;

  @tracked declare newModel: Partial<ComponentModel>;

  constructor(owner: unknown, args: MyComponentArgs) {
    super(owner, args);
    this.createNewModel();
  }

  createNewModel() {
    let { relatedModel } = this.args;
    this.newModel = this.store.createRecord('component-model', {
      relatedModel, // if I remove this, we don't get the error
    });
  }
}

Is this method of creating a new model within a Component no longer supported? Or is this a bug? 🤔

[runspired]

Most definitely this error is a bug.

[runspired]

was hoping to fix this in the next 4.7 patch but I don't have a good test for it. I have some suspicions around what might be triggering it (some of which are sadly unfixable as they are limitations of how tracking works in Ember unless Ember were to directly expose untracked used by computed as a primitive for us to build with). I'll see if I can find one of the failures in our app and use it to create a failing test.

[runspired]

Most of the instances of this in our app are around arrays, not createRecord specifically (though it sometimes may be a createRecord where a peekAll is also in use). @kpfefferle @alexraputa does that align with your experience?

[kpfefferle]

@runspired Ours is not a createRecord with peekAll, but it is a createRecord that is associating the new model with an existing model that has been passed into the component via this.args. If I remove the relationship from createRecord, I do not get the error. I've updated the above code sample to reflect this aspect of our failure case.

[runspired]

@alexraputa @kpfefferle yeah I'm no longer convinced fully this is a bug in EmberData. The backtracking that's happening here is real and its the pitfall of trying to mutate app-wide state within a component.

I think what has changed is that EmberData no longer uses computed properties (which "untrack" changes) for many things, combined with we've eliminated many inefficiencies where we were previously doing setup "async" (not actually) via a second pass of the run-loop.

A solution is to make createRecord actually async, it's something I've considered doing before. In fact many of the proposed design directions for the store convert all or most API's to async for these sorts reasons. However both of these things are breaking changes in a much larger sense.

[kpfefferle]

@runspired I can understand how a component associating a new record with an existing one could reasonably be understood as mutating the related component in a way that invalidates it in a wider context (and in this case, as an argument to the component itself). However, the use case seems to be a common one: the user has clicked a button that opens a form to create a new model related to a given existing model.

Do you have a suggestion for how ember-data users should accomplish this goal without triggering this error? Should we preemptively create the new model in the route (even if the user may never click on the button to open the form)?

[runspired]

@kpfefferle I typically create it as part of the click action to open the modal, or if I prefer to buffer I create it on the click action to submit the modal. In both of these cases you will avoid any backtracking.

[kpfefferle]

@runspired We literally only have one component in our application that's doing this, so I'll give those approaches a shot to get us back on the latest stable train 👍

[runspired]

All this said I am poking at this to see if there's something simple I can do. I don't want to go back to two pass, but maybe I can defer the relationship and record-array notifications with a deprecation that you resolve via a special flag (since we can't possibly detect this ourselves). Along the lines of the sync=>async observer switch over.

[alexraputa]

@runspired, this code also cause the deprecation:

import { inject as service } from '@ember/service';
import Component from '@glimmer/component';

export default class Checkout extends Component {
  @service store;

  order;

  constructor(owner, args) {
    super(owner, args);

    this.order = this.findOrder();
  }

  findOrder() {
    let order;
    const orders = this.store.peekAll('order');

    if (orders.length !== 0) {
      order = orders.find((found) => found.isNew);
    }

    return order ?? this.store.createRecord('order');
  }
}

[runspired]

Yeah. That's where folks using the resource pattern are regularly adding await Promise.resolve() to sever the notifications from the tracking context. It's frustrating because it's clearly not an error here but it also is. untracked would be a good thing here. I pinged @wycats for some general thoughts. I'm increasingly finding way too many of these footguns with tracked as folks convert more code out of computeds.

[runspired]

One thought, the actual primitive needed here is a tracking transaction. While untracked would work for the case above, you might find yourself in a similar situation in which you need to read-before-modify but still want to subscribe. This is in fact exactly the case for resources where I'm seeing folks bash their heads against the wall, and it's something where I've had to repeatedly write complicated work-arounds for code very similar to findOrder in the example above.

An example where folks are regularly hitting failures are things like query, where you end up with a length subscription when store.query is called that mutates when the request resolves re-triggering the initial code which is now unnecessarily subscribed.

Since EmberData has fairly good control of its tracking updates, I could probably create such a transaction primitive specific to ours to silently dirty props and only notify ref after a transaction. That said, it would be better if this was built into @glimmer/tracking.

A rough API for this might be

import { transact } from '@ember-data/tracking';


function findOrder(store) {
    return transact(() => {
      let order;
      const orders = store.peekAll('order');
  
      if (orders.length !== 0) {
        order = orders.find((found) => found.isNew);
      }
  
      return order ?? this.store.createRecord('order');
    });
}

Though a memoized version might be nicer:

import { memoTransact } from '@ember-data/tracking';

const findOrder = memoTransact((store) => {
   let order;
    const orders = store.peekAll('order');

    if (orders.length !== 0) {
      order = orders.find((found) => found.isNew);
    }

    return order ?? this.store.createRecord('order');
})

[runspired]

I'm also curious if @developit has given any thought to these sorts of issues for Signals (I'm happy to give a more abstract summary if you can't gather from context above of what I'm asking). I want to bring EmberData to the React/Preact community early next year and I'm sure we'll hit this sort of problem there as well.

[alexraputa]

transaction primitive

It would be nice to have API like that.

That said, it would be better if this was built into @glimmer/tracking.

@wycats, maybe @starbeamjs have the similar API?

[runspired]

Impl for data: c1e058c#diff-e007ce9be17cc346c9a985014647c88c62a07f706d18a74eb813de2cd9b813c7

[runspired]

@alexraputa @kpfefferle I have a spike of transact working in this PR: #8214

The public API for it will be exactly as I showed above, and I'm shipping it with typescript support 🎉

[runspired]

Closing as tracking lib has shipped

[bartocc]

I hit this bug when migrating to ember-data@4.7 as well.

I believe our use case is slightly different than what has been previously
described in this issue, but we do hit the bug because we use store.peekAll and
store.createRecord.

In RouteA, we render ComponentA
When rendered, ComponentA calls store.peekAll("model-a")

We then transition to RouteB, which renders ComponentB, but not ComponentA.
RouteB is not nested under RouteA.
When rendered, ComponentB (a form) calls store.createRecord("model-a").
And we hit the bug.

I've worked around this regression by buffering the creation of "model-a" in ComponentB with a proxy.
store.createRecord("model-a") is now only called when submitting the form.
This is what @runspired suggested in #8173 (comment)

I also tried to import untracked from @ember-data/tracking, and wrap the
call to store.peekAll("model-a") in ComponentA with it, but this did not fix the
regression.

I should add that our app uses an in-repo-addon and that ComponentA and ComponentB are part of this addon.

@runspired, do you believe our use case is not supported by @ember-data/tracking?

[runspired]

We then transition to RouteB, which renders ComponentB, but not ComponentA.
RouteB is not nested under RouteA.

the way ember/glimmer works, this should not result in this issue if this is in fact true, which suggests to me that this is not true or some other thing also consumes the state.

I don't know that I'd use a proxy to buffer the creation, but I wouldn't call createRecord via a component constructor ever, as that doesn't really make sense. You want to change state on user action or during navigation, not during render.

[bartocc]

We then transition to RouteB, which renders ComponentB, but not ComponentA.
RouteB is not nested under RouteA.

the way ember/glimmer works, this should not result in this issue if this is in fact true, which suggests to me that this is not true or some other thing also consumes the state.

I agree, this is strange!

But when we directly access RouteB via its URL, we do not hit the bug…
which suggests we do not consume the state in the template rendered with RouteB

I am out of ideas here 😊

I don't know that I'd use a proxy to buffer the creation, but I wouldn't call createRecord via a component constructor ever, as that doesn't really make sense. You want to change state on user action or during navigation, not during render.

How would you buffer the creation then? I'd like to add you idea of the implementation to my choices. Thx 🙏