_updatePromiseProxyFor throws "used previously in the same computation" error

[wongpeiyi]

When an API payload is received with an association not already set on the Ember model, the following error is seen:

Error: Assertion Failed: You attempted to update `content` on `<(unknown):ember173>`, 
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.

`content` was first used:

- While rendering:
  
  application
    foo
      this.dataLength

Stack trace for the update:
    at dirtyTagFor (validator.js:514)
    at markObjectAsDirty (index.js:637)
    at notifyPropertyChange (index.js:675)
    at set (index.js:1622)
    at Proxy.set (observable.js:176)
    at InternalModel._updatePromiseProxyFor (-private.js:4379)
    at InternalModel.getBelongsTo (-private.js:4272)

This happens when the model is being consumed in a component template (via a getter) that depends on post.user

Reproduction

https://github.com/wongpeiyi/data-computation

Description

• When creating and saving a Post, no User association is set yet
• The API (mirage) sets a User and responds with a payload
• Ember Data updates the Post model
• Because the model is being consumed by the component getter that depends on filterBy('user.id'), the error is thrown

The error is not thrown when:

• Not filtering (or not consuming the user association)
• Using {{@data.length}} in the template instead of the component getter
• Using a getter on the controller without a component

This seems like a use case that should "just work"

Versions

Run the following command and paste the output below: yarn list ember-source && yarn list ember-cli && yarn list --pattern ember-data.

yarn list v1.19.2
warning Filtering by arguments is deprecated. Please use the pattern option instead.
└─ [email protected]
✨  Done in 0.72s.
yarn list v1.19.2
warning Filtering by arguments is deprecated. Please use the pattern option instead.
└─ [email protected]
✨  Done in 0.65s.
yarn list v1.19.2
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ [email protected]
└─ [email protected]
✨  Done in 0.63s.

[runspired]

This looks like an interop issue with tracked properties and mirage. Don't have a ton of time to dig in but I'd guess the issue is related to mirage being an unrealistic async environment.

[wongpeiyi]

@runspired thanks for the response. Unfortunately, that's not correct – I took the time to create this minimal reproduction cause I thought it would help, but it was an error I experienced in production with a live API.

[snewcomer]

Since args are autotracked and the Proxy.set uses Ember.set, it seems like this is the cause for the error in the same computation ("render").

ref emberjs/ember.js#18668

A few thoughts.

1. Since you put such a detailed application and commits together (stacktrace is from ember.js), it might be worth filing with the ember.js repo!
2. This also might be a good opportunity to refactor the code to avoid it and come out better on the other side. Although less flexible, if an improved codebase also solves the problem then that might have to be good enough if there is no movement on 1). Also - looks like you aren't interopt-ing computed and tracked in your app code (which can lead to subtle bugs) 👍

[wongpeiyi]

Thanks @snewcomer I'll file it with ember.js

This also might be a good opportunity to refactor the code to avoid it and come out better on the other side.

Do you have any ideas about how to refactor / avoid this error? The only way I can think of is to perhaps enforce more precise timing in the data retrieval from the store, but that is not ideal

[snewcomer]

The one piece that feels a little off track is the store.peekAll().filterBy() logic in the Controller. By creating a new record in a Component (child), you are implicitly relying on the EmberArray observability. If you had say, store.peekAll().toArray().filterBy() in your Controller, you won't get the reactivity b/c you cut out the EmberArray observability.

In this new world of Octane, working with pure pull (tracked) semantics rather than push (observability) is the recommended way. I would personally make it all less magic. I would first look at ensuring this implemented data-down-actions-up properly. So creating the new Post would explicitly send the data back down. Here is what it might looks like...

// this could be the Controller or Component
import Controller from '@ember/controller';
import { action } from '@ember/object';
import { tracked } from '@glimmer/tracking';
import { inject as service } from '@ember/service';

export default class ApplicationController extends Controller {
  @tracked data = [];
  @service store;

  init() {
    super.init(...arguments);

    this.data = this.storeData;
  }

  get storeData() {
    // filter works as well as long as mirage returns a user
    return this.store.peekAll('post').toArray();
  }

  @action
  async createPost() {
    await this.store.createRecord('post').save();
    this.data = this.storeData;
  }
}

// template only
// foo.hbs
{{@data.length}}

[wongpeiyi]

Thanks @snewcomer on one hand I understand the principles behind this, but on a practical level I struggle with this a little. When you say:

By creating a new record in a Component (child), you are implicitly relying on the EmberArray observability. If you had say, store.peekAll().toArray().filterBy() in your Controller, you won't get the reactivity b/c you cut out the EmberArray observability.

I'm not sure I understand this. AFAIK filterBy takes the DS.RecordArray and already returns an EmberArray, so toArray() doesn't nothing here.

Also, what's wrong with relying on EmberArray "observability"? Isn't using a tracked EmberArray (e.g. https://guides.emberjs.com/release/upgrading/current-edition/tracked-properties/#toc_arrays) relying on that same "observability"? P

Practically speaking, I'd expect DS.RecordArray to behave like a tracked EmberArray. If the records change, I'd expect any dependent getters to update. Here you are essentially saying we cannot treat RecordArrays like tracked properties – instead you have to manage the updating yourself. This defeats the whole point of a RecordArray with update(), isUpdating, isLoading methods in the first place.

---

The reason I posted this here instead of in ember.js is because I thought it was a mismanagement of _updatePromiseProxyFor (as per the title):

data/packages/store/addon/-private/system/model/internal-model.ts

Line 828 in e154539

promiseProxy.set('content', args.content as any);

This is from the stack trace.

From my limited knowledge, this seems like it's happening because on render, the filterBy calls the Post model's getBelongsTo, which calls the above and sets the content of the PromiseProxy at the same time it is being rendered.

That seems like an ember-data issue?

[snewcomer]

You make a lot of good points.

RecordArray's ArrayProxy content is an EmberArray that implements MutableArray and Observable. However, filterBy's return object (the visible piece to autotracking I think) does implement Observable whereas RecordArray does not. Thus w/ filterBy, @data implements MutableArray and Observable. With RecordArray, it is just implements MutableArray.

Observable is inherently a push based system whereas autotracking is a pull based system. As a result, we live in a touchy world where we are implementing both. My preferred solution would be to implement one or the other and as you have shown, it might not be obvious.

so toArray() doesn't nothing here.

This was just to illuminate that I generally would move towards plain arrays if my use case afforded it (explicit DDAU w/ simple iteration).

[wongpeiyi]

Thanks @snewcomer for explaining. I'll definitely have to read more into how tracking works.

I've encountered this error in several other situations since, and unfortunately a production app is typically way too complex to be able to implement this kind of solution. Records are often not created / updated where they are rendered. Having a "single source of truth" Store is pretty much the point here – having to maintain tracked arrays everywhere defeats that purpose.

However, if it was absolutely necessary to do that, I think the deprecated didLoad, didUpdate lifecycle events could have helped here. I'm not sure if that counts as "push" or "pull", but they would help to maintain a tracked array that reliably tracks the store records. It seems they were deprecated in favour of computed properties, but that's not available in Octane either? So the general feel is that we have no tools left: model lifecycle events are deprecated, observers are evil, computed properties are the old paradigm, and now using getters to track store records is unreliable as well.

I've resorted to "de-syncing" the store fetches with the renders by adding 1ms timeout to fetch tasks. Not ideal, but prevents us from having to downgrade Ember.

[runspired]

I spent some time today digging into this and I believe we should close this as "user error" (don't worry, I will explain below!). The issue was certainly tricky to spot, and @pzuraq may have some thoughts on whether there are any action items on behalf of the framework or learning team to make this less painful in the future.

Root Cause

On the controller we define an unstable getter. "Unstable" because every access of this getter will return a fully new array instance.

get data() {
  return this.store.peekAll("post").filterBy("user.id", "1");
}

In the template, we render some values based on data. Templates memoize the return value of the getter, so accessing data multiple times appears to work.

But then we added this getter in a component which receives this data.

  get dataLength() {
    return this.args.data.length;
  }

At this point an error is thrown if dataLength is consumed during render. Note, the error is thrown even if length is not accessed, just accessing args.data is enough.

Here's the catch, passing the data prop via a template does not pass the value, but rather a wrapper that maintains the calling context. So accessing args.data on when passed to a component like below will retrigger the getter.

<Foo @data={{this.data}} />

Simple Fix

Memoize the getter.

  get posts() {
    return this.store.peekAll("post");
  }

  @computed("posts.length")
  get data() {
    return this.posts.filterBy("user.id", "1");
  }

More Complicated Considerations

• can we detect unstable getters and improve the error message when this happens?
• can we somehow memoize the value passed through to a component as an arg?
• can we improve learning materials to help explain how to detect and handle these cases?

[runspired]

After even deeper investigation I learned a few more things:

1. In this particular reproduction, the use of mirage does make things worse as it flushes a change to the promise proxy that should have been async (network request) within the same render as initiated the request. This is what I initially expected to be the issue, and was only revealed once the issue in Ember was fixed here: [BUGFIX lts] dont access properties during init unless required ember.js#19023

2. GlimmerVM is doing less memoization in templates than I realized. While individual property accesses are memoized, accessing the same property multiple locations within the same template is not. It is possible to trigger this error from within a single isolated component by just having an unstable getter used multiple places in a template. There's a simple optimization glimmer could do here that would dramatically reduce property accesses in these sorts of scenarios, which is to merge all PropertyReferences for the same property in a way in which they share a cache.

A simple user-land example of this would be to use let to access data in the template and using the let version throughout. However this has the downside of eagerly computing the getter when it may not be used if it is passed into a child component as an arg. An ideal solution merges the property references but remains lazy on the calculation.

[runspired]

I created glimmerjs/glimmer-vm#1111 to help out here a little bit, it seems from what I can tell glimmer-vm does do a better job memoizing in production.

[wongpeiyi]

@runspired thanks, I'm obviously in over my head here when in comes to the internals, but I would like to understand the "simple fix" at least as an end user of the framework

When you say to memoize the getter, do you mean that the usage of @computed together with getters are a recommended pattern?

And in what situation is this required to avoid the "user error" here? Is it: "do not use getters when dealing with ember-data RecordArrays?", "use @computed when a getter takes a RecordArray and returns an EmberArray", etc. I'm not sure what the takeaway "rule" is as an end user, or how exactly to address this in my app

As mentioned, the use of mirage was only to demonstrate the error, but I was facing this all over my live production app when I upgraded from 3.14 to 3.18. If this is to be closed as "user error" I'd like to know what exactly the user error I made was, and what changes I have to make to my codebase exactly

It's not obvious to me how

get data() {
  return this.store.peekAll("post").filterBy("user.id", "1");
}

is an erroneous use of the framework...

[snewcomer]

@wongpeiyi This fix was backported to 3.16.9. Mind checking to see if this error still is around?

closed here - emberjs/ember.js#19003

[runspired]

@snewcomer it likely is still around due to the memoization issue with DEBUG templates in glimmer. I need to add another test to the PR so it gets merged.

@wongpeiyi getters run on every access, so if they return a referentially unstable value (eg they make a new computation each time) then accessing that getter more than once in a render is an error. Similarly this is why in JS code the best practice would be to cache the value of the computation for reuse vs recalculating it every time you need access to it again.

[runspired]

Closing this issue as there is nothing more for us to do but the changes in #glimmerjs/glimmer-vm#1111 are still relevant.