Computed properties dependent on async relationships do not recompute when the relationship fulfills

[kategengler]

Reproduction

Reproduced here https://github.com/kategengler/datatest With branches for 3.27 (working) and 4.2.0-beta.0 (not working)

Description

A computed property dependent upon an async relationship does not recompute when the relationship fulfills.

In the reproduction, post hasMany (async) comments which hasMany (async) authors

https://github.com/kategengler/datatest/blob/main/app/models/post.js#L11-L18 is the computed property, it is referenced in application.hbs where the model is a single Post.

In the console, you will see that the comments are fetched and the authors computed property is only run once, during which comments.isFulfilled is false.

If you uncomment comments.isFulfilled as a dependent key on the CP, you will see it run twice, once when it is fulfilled, and authors are fetched via the API, but still the authors do not list in the template.

In the template, you can change to using authorsTracked and see both authors fetched and displayed in the template.

Similarly, you can switch to branch ember-data-327 and see expected behavior while using the CP. Branch ember-4 uses ember-data 4.2.0-beta.0 and has the same unexpected behavior as 3.28

Versions

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

└─ ember-source@3.28.8
✨  Done in 0.54s.
yarn list v1.22.17
warning Filtering by arguments is deprecated. Please use the pattern option instead.
└─ ember-cli@3.28.5
✨  Done in 0.54s.
yarn list v1.22.17
├─ @ember-data/adapter@3.28.8
├─ @ember-data/canary-features@3.28.8
├─ @ember-data/debug@3.28.8
├─ @ember-data/model@3.28.8
├─ @ember-data/private-build-infra@3.28.8
├─ @ember-data/record-data@3.28.8
├─ @ember-data/rfc395-data@0.0.4
├─ @ember-data/serializer@3.28.8
├─ @ember-data/store@3.28.8
├─ babel-plugin-ember-data-packages-polyfill@0.1.2
└─ ember-data@3.28.8

[bmfay]

@kategengler we encountered widespread issues after upgrading to 3.28. Can confirm it is the same issue with computed properties on async relationships and that it worked through 3.27 and no longer works in 3.28. It is definitely Ember Data since our CP's work fine with Ember CLI 3.28 and Ember Data 3.27.

I see nothing in the release notes indicating any intended changes around this. Would be great to hear from the team on this!

[robinborst95]

Same for our codebase, I ran into this same issue and actually resolved it by adding dependent keys like comments.length everywhere (don't know if that is relevant, but at least comments.isFulfilled is not the only dependent key that 'fixes' it).

For normal CPs adding an extra dependent key to work around this issue was relatively doable, but this issue also affect computed macros. For example @filterBy('comments', 'isDeleted', false) visibleComments; does not work either, so I had to replace such macros with actual CPs and do this.comments.rejectBy('isDeleted') manually (+ the extra dependent key).

One way to work around this issue is turning all classic code with computed properties into Ember Octane with tracked properties etc, but we still have hundreds of components, models, controllers, etc that use computed properties, so converting those takes time. Until we manage to have everything converted, this issue is blocking for us, so we'd like to hear from you too!

[kategengler]

@robinborst95 In my case, adding .length dependent keys did not help.

[robinborst95]

Hm, that is strange :thinking: We also use ember-source 3.28 and for us it does work. I also tried your reproduction repo and both comments.isFulfilled and comments.length make the CP update properly actually.

[frederikbosch]

@kategengler Did you find a solution? Adding .length does not help for us too.

[frederikbosch]

My solution is to access .isFulfilled inside the computed property.

@computed('replies.@each.id')
get numReplies() {
  let _ = this.replies.isFulfilled;
  return this.hasMany('replies').ids().length;
}

[kategengler]

I found using .isFulfilled worked, but the problem was too extensive, so I downgraded to ember-data 3.27. An alternative solution is migrate to @tracked

[runspired]

I think the underlying issue here is that @each chains and the computed macros (which relied on private observer-based ember-api's to receive change notifications of array-proxies) don't subscribe to [] or length on the array-proxy. This is I believe a bug in ember, as those things are no longer supposed to rely on the observer api's (which were deprecated and part of our motivation for deleting in 3.28). It looks like internally they are still living strong.

[runspired]

Found a fix, I think the underlying issue is computed chains check length but don't recompute based on it, they need the fake [] property still.

[robinborst95]

@kategengler @runspired I've tried out the 3.28.10 patch release and it does work for @each dependent keys. However, I've run into the same issue when firstObject/lastObject is used as dependent key, so I wonder if this can be confirmed and if so, whether a similar fix for this can be released soon :pray:.

To demonstrate this, I've forked the same datatest repo and added an example: https://github.com/robinborst95/datatest. Similarly, I added a downgrade-327 branch to show that it did work properly on 3.27.

Adding @dependentKeyCompat to the getters here fixed it for me locally at least, but I don't know if it has side-effects. I also don't know why it wasn't added to these getters in the first place, so I'm asking about it here first before making a PR with the fix + test.

Edit: I've tried to adapt the test added by #7945 in a way that also firstObject/lastObject is tested, but for some reason that didn't fail. I tried @readOnly('members.lastObject') lastMember, add a member with group.members.pushObject(...) and then let lastMember = group.lastMember to use in assertions, but that returned the expected added member. However, you can clearly see in the screenshot that the template wouldn't update, so what could be the explanation for that?