adopted-ember-addons / ember-data-model-fragments

Ember Data addon to support nested JSON documents
MIT License
370 stars 114 forks source link

model.reload() don't update fragmentArray properties (using v6+ and ember-data 4.6) #467

Closed ndekeister-us closed 1 year ago

ndekeister-us commented 1 year ago

Hello!

First thanks a lot for the awesome work to make this repository compatible with Ember 4+ 🙏

With version 6 of this addon and ember-data 4.6 we found a little issue, when using model.reload(), fragmentArray properties are not updated correctly

https://github.com/adopted-ember-addons/ember-data-model-fragments/assets/56396753/f18349d0-72d3-4e20-9520-1889b74784cf

If we are looking at model.currentState.recordData.__fragmentData['myProperty'], it contains correct values, but model['myProperty'] still contains the initial value

Reproduced here -> https://github.com/ndekeister-us/ember-test-app_ember-data-model-fragments

deanmarano commented 1 year ago

I've also experienced reload not working for fragmentArrays on ED 3.28. It looks like this test for reloading is skipped, and it is failing if ran.

deanmarano commented 1 year ago

I checked out the sample repo here and the PR that I got merged doesn't resolve this issue (because of this rollback). This code is the issue (even though I previously thought it might not be necessary to change):

if ((updates[key] === null) !== (original[key] === null)) {
  changedKeys.push(key);
}

It handles if the fragment is null or is becoming null, but doesn't actually detect changes (as the _changedFragmentKeys function name implies).

I'm not sure what the best way to detect changes here. My initial attempt was checking if they === each other, which doesn't work for arrays (so it was always triggering, which did fix the bug). I considered verifying on length and clientId:

let wasOrBecameNull = original[key] === null || updates[key] === null;
let isDifferentLength = original[key]?.length !== updates[key]?.length;
let hasNewIds = !updates[key]?.filter(record => original[key]?.includes(record.clientId));
if (wasOrBecameNull || isDifferentLength || hasNewIds) {
  changedKeys.push(key);
}

I've tested this on the provided reproduction (thanks @ndekeister-us!) and it works (as well as a repro in my app), but I haven't written a test for it yet.

deanmarano commented 1 year ago

@ndekeister-us Can you verify that the latest commit (not released package) works for you?

knownasilya commented 1 year ago

Released as v6.0.2

ndekeister-us commented 1 year ago

Thanks a lot for the fix + release, tested on our main application and on the demo app and it is now working as expected 🎉