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

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

Changes to fragment array are not present in the owner record's changedAttributes #330

Open nhemanth007 opened 5 years ago

nhemanth007 commented 5 years ago

changedAttributes is not returning changes in fragment array on the model.

Ember version used 3.8 Addon version used is v4.0.0

I forked this addon(v4.0.0) and added a unit test case to verify the issue. The test case is not complete, I asserted owner record's changedAttributes with empty string to know what value is returned.

Value returned by changedAttributes is not appropriate. Old property is a fragment object. But new property is a simple object.

You can find the corresponding test case in https://github.com/nhemanth007/ember-data-model-fragments/commit/44ad92d14c92ca63cd528f4f08fbb6295d2837f2

names is a fragment array on person model. String version of data returned by person.changedAttributes().names is [{"type":"name","options":{},"name":"names","owner":{"title":null,"nickName":null,"name":null,"names":[{"first":"Cat","last":"Stark"}],"addresses":[],"titles":[],"hobbies":null,"houses":[],"children":[],"strings":[],"numbers":[],"booleans":[]},"_objectsDirtyIndex":-1,"_objects":[{"first":"Cat","last":"Stark"}],"_lengthDirty":false,"_length":1,"_arrangedContent":[{"first":"Cat","last":"Stark"}],"_originalState":[{"first":"Cat","last":"Stark"}]},{"first":"Cat","last":"Stark"}]

nhemanth007 commented 5 years ago

Names on person are changed from names: [{ first: 'Catelyn', last: 'Stark' }] to names: [{ first: 'Cat', last: 'Stark' }]

mtgraham commented 5 years ago

I'm seeing a similar issue with my fragmentArrays. I make a change to one of the fragments in the fragmentArray and the parent model will return true for hasDirtyAttributes, but changedAttributes() returns an empty object. Also, it seems that sometimes my model will enter a state where parent.hasDirtyAttributes === false and parent.fragmentArray.hasDirtyAttributes === true.

VincentMolinie commented 4 years ago

Try with the new beta version I think it might fix your issues 😉

VincentMolinie commented 4 years ago

I actually do have the same issue than @mattgraham1995 on version 5.0.0-beta.0

mpirio commented 3 years ago

I have the same "bug" : make a change in a fragmentArray (add or remove an item), model.hasDirtyAttributes is true but changedAttributes() is empty.

I find fragmentDidDirty and this line in it :

record._internalModel._recordData.setDirtyAttribute(key, fragment);

But in setDirtyAttribute, ember-data set _attributes[key] line 401 with my fragmentArray value, set originalValue with the same fragmentArray value and finally compare _attributes[key] and originalValue so unset _attributes[key]. So changedAttributes() will always be empty!

In states.js history, I find this change:

-    record._internalModel._recordData._attributes[key] = fragment;
-
+    record._internalModel._recordData.setDirtyAttribute(key, fragment);

If I revert this change, the bug is resolved... I think the ember-data recordData.setDirtyAttribute is not adapted for fragment.

@cohitre What do you think about it? Perhaps should we revert this change? If OK, I can make a PR.

mpirio commented 3 years ago

An Ember Twiddle to see the bug: https://ember-twiddle.com/93d84d0a212054ff2a0fc146b27917ae

mpirio commented 3 years ago

I have a fix-issue-330 branch in my repo but I can not PR to this repo because I need a "release-4.0.0" branch here to PR into it. Thanks.

stfnio commented 3 years ago

@jakesjews by any chance, do you know why the "release-4.0.0" branch has disappeared? I need the fix discussed above to be merged, as well

mpirio commented 3 years ago

No news ?

I try a new call :) @jakesjews @araddon @cohitre @markhayden. Is it possible to create a "release-4.0.0" branch to PR a fix about this issue?

Thanks.

jakesjews commented 3 years ago

@mpirio were currently in progress moving this repository to ember-adopted-addons https://github.com/lytics/ember-data-model-fragments/issues/382

jakesjews commented 3 years ago

If you need the changes right now your best bet is making a fork and branch from commit f1c7060fb7560aaea26760e0270234823b9e20db

mpirio commented 3 years ago

Hello @jakesjews. Thanks for your message.

@mpirio were currently in progress moving this repository to ember-adopted-addons #382

Yes I read about adopting this addon. It's a good news. But does the procedure block bugfixes on this repository?

If you need the changes right now your best bet is making a fork and branch from commit f1c7060

That's what I did :). But wouldn't it be better to centralize bugfixes here?

Regards,

jakesjews commented 3 years ago

Yes it would but there is some more to the problem as well. I can't actually merge anything right now. I don't really have much time to work through all the stuff hence moving the repo to adopted-addons.

mpirio commented 3 years ago

Hello,

It seems the migration to adopted has been made. Really good 👍 . Maybe can I give you my fix for this issue? Would it be possible to create a release-4.0.0 branch so that I can push a PR? Thank you.

knownasilya commented 3 years ago

You can submit a PR against the master branch, no need for another branch.

mpirio commented 3 years ago

Hello,

Thank you for you message. This PR is for v4.0.0 not 5.0. See https://github.com/mpirio/ember-data-model-fragments/tree/fix-issue-330

knownasilya commented 3 years ago

https://github.com/adopted-ember-addons/ember-data-model-fragments/tree/release-v4