emberjs / ember.js

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

[Bug] Ember Data Relationship Changes Not Triggering did-update Modifier #19189

Open jrjohnson opened 4 years ago

jrjohnson commented 4 years ago

🐞 Describe the Bug

in Ember 3.19.0 the following usage of did-update would work. It stopped working in 3.20 and continues to not work in 3.22 even with fixes for #19105 and fix in #19164 and with Ember Data 3.22.0.

<div
  {{did-insert this.load}}
  {{did-update this.load @post.comments}}
>
</div>

🔬 Minimal Reproduction

Reproduced in https://github.com/jrjohnson/reproduction-did-update-relationship Steps: 1) npm start 2) Visit http://localhost:4200/tests?devmode 3) Open Console 4) Clicking Add POSTs a new record and re-computes. 5) Clicking Clear PATCHs and removes the records, but does not re-first did-update.

😕 Actual Behavior

did-update no longer fires whenever a records are removed from a relationship.

🤔 Expected Behavior

did-update should fire whenever changes are made to the relationships it is provided.

🌍 Environment

➕ Additional Context

I've contrived an example using mapBy here, but anything that doesn't use the PromiseArray to track the value will fail (eg post.comments.toArray()). A workaround is to set PromiseArray to a local value and then use a getter to transform it, but that doesn't work in more complicated cases where many async relationships need to be combined to render a final value.

I know that @snewcomer and @pzuraq have been working on the fixes for this issue so I'll tag them here as well.

snewcomer commented 4 years ago

https://github.com/emberjs/data/pull/7330

Ref PR that I think fixes it in ED for now.

jrjohnson commented 4 years ago

Ah ok, I was under the impression that there was no new code there (just tests) and everything else had landed in Ember 3.22.0 already.

jrjohnson commented 4 years ago

I npm linked sn/has-many-consume from emberjs/data#7330 and it doesn't seem to fix this issue.

snewcomer commented 4 years ago

@jrjohnson Now that we have e-d 3.22 released, mind doing a double take just to be certain?

jrjohnson commented 4 years ago

Double checked with ED 3.22.0 and added that to my reproduction, issue persists unfortunately.

snewcomer commented 3 years ago

Looking at this again, I was surprised this worked before (but obviously you showed it did). I didn't think did-update could be reactive to the length of @post.comments. If @post.comments was a new array, I would imagine this would work. However, we have the same reference before and after adding/deleting a comment.

Overall, I think this is probably a bit outside my wheelhouse to help tackle.