ember-data-model-fragments icon indicating copy to clipboard operation
ember-data-model-fragments copied to clipboard

Computed properties won't update on changes to nested model fragements

Open benoror opened this issue 8 years ago • 17 comments

I have the following models:

// app/models/user.js
import Model from 'ember-data/model';
import {
  fragment
} from 'model-fragments/attributes';

export default Model.extend({
  family      : fragment('family')
  // other attrs
});
// app/models/family.js
import Model from 'ember-data/model';
import {
  fragment
} from 'model-fragments/attributes';

export default Model.extend({
  spouse      : fragment('personal'),
  mother      : fragment('personal'),
  father      : fragment('personal')
  // other attrs
});
// app/models/personal.js
import attr from 'ember-data/attr';
import Fragment from 'model-fragments/fragment';

export default Fragment.extend({
  firstName : attr('string'),
  lastName  : attr('string'),
  activeInsurance  : attr() // This is not 'boolean' as it can be three-state: null, true & false
});

If I have an instance of the user.family model (let's say passed into a component), and I have a computed property which observes that model, cached result of the computed property won't be updated when the nested fragments data is changed:

// This computed property won't react to changes on any family member
allFamilyHasInsurance: computed('myUser.family', function() {
  //...
}),

benoror avatar Nov 29 '16 06:11 benoror

I just found that JSONAPI is not fully supported:

Since fragment deserialization uses the value of a single attribute in the parent model, the normalizeResponse method of the serializer is never used. And since the attribute value is not a full-fledged JSON API response, JSONAPISerializer cannot be used with fragments. Because of this, auto-generated fragment serializers do not use the application serializer and instead use JSONSerializer.

https://github.com/lytics/ember-data-model-fragments/blob/master/README.md#serializing

Could that be the root of the issue?

benoror avatar Nov 29 '16 06:11 benoror

Could you push up a repository with a reproduction of the issue so I can debug it?

jakesjews avatar Nov 29 '16 19:11 jakesjews

+1

douglascofferi avatar Dec 19 '16 10:12 douglascofferi

+1

catataw avatar Dec 19 '16 20:12 catataw

@catataw @douglascofferi Do either of you have a repo reproducing the issue so I could debug it? I haven't encountered the same behavior yet.

jakesjews avatar Dec 20 '16 03:12 jakesjews

@jakesjews sorry for the delay, I've been pretty busy lately 🙈 I will try to reproduce it in a separate repo asap, maybe this week. Cheers!

benoror avatar Dec 20 '16 15:12 benoror

@jakesjews yes bro, i have the following models:

// app/models/justice.js
import { fragment, fragmentArray, array } from 'model-fragments/attributes';
import Model from 'ember-data/model';
import attr from 'ember-data/attr';
import ValidationJustice from 'adv2c/validations/justice';

export default Model.extend(ValidationJustice, {
  judgment: fragment('justice/judgment'),
  // other attrs
});
// app/models/justice/judgment.js
import attr from 'ember-data/attr';
import Fragment from 'model-fragments/fragment';

export default Fragment.extend({
  // other attrs
  courtName: attr('string'),

  courtNameProp: Ember.computed('courtName', function() {
    console.log(`${this.get('courtName')}`);
    return `${this.get('courtName')}`;
  }),
  //With observer also does not work
  courtNameChanged: Ember.observer('courtName', function() {
    console.log(`courtName changed to: ${this.get('courtName')}`);
  })
});

Thank you man!

douglascofferi avatar Dec 21 '16 11:12 douglascofferi

@douglascofferi do you have an example of how the computed property is being consumed?

jakesjews avatar Dec 21 '16 14:12 jakesjews

@jakesjews In that case my computed property works, however I have a need to create an Ember.on ('init', Ember.observer inside a fragment and this does not work. Any special reason? Thank you for your help.

douglascofferi avatar Dec 22 '16 10:12 douglascofferi

it looks like you are hitting an exception resolved by https://github.com/lytics/ember-data-model-fragments/commit/debe667d72f560b34bda32aff90600e914c72b53. Unfortunately we still have to coordinate to get NPM publish permissions but for now installing the 2.3.3 tag from git should fix the issue.

jakesjews avatar Dec 22 '16 14:12 jakesjews

@douglascofferi @jakesjews interesting... although my initial question was more about the possibility of observing the whole fragment for internal changes.

In @douglascofferi example that would be observing judgment for changes.

Is it possible? Cheers!! 😄

benoror avatar Dec 30 '16 17:12 benoror

@benoror When you say "observing the whole fragment for internal changes", what exactly do you mean? When the fragment becomes dirty/clean? When values are updated? When new changes are pushed in from the server? It sounds like all of the above?

I typically try to observe individual properties that I'm dependant on, rather than trying create an observer for every change that could be made.

Maybe you could tell me a little more about your use case.

workmanw avatar Dec 30 '16 19:12 workmanw

Hi @workmanw, thanks for asking. I meant "When values are updated". My use case is more or less like this:

I have several objects (fragments) with a bunch (lots!) of different properties (keys). Those are rendered dynamically using ember-formly.

It would be overkill & tiresome to create computed properties/observers for every property, thus I would like to be able to detect any internal change.

Hope that clarify my intentions.

Cheers and happy new year! 🎉 👏

benoror avatar Jan 01 '17 02:01 benoror

@benoror you should be able to use a combination of the fields property and dynamically defined properties as a solution

jakesjews avatar Jan 01 '17 23:01 jakesjews

@jakesjews thanks for the info! The second article is indeed interesting, although more inclined towards collections (arrays).

For plain objects/fragments I was thinking about using raw observers:

  init() {
    this._super();

    // fragment instance object, not model
    // although 'fields' property from Model might be used as well
    let fragment = get(this, 'fragment');

    fragment.eachAttribute((key) => {
      fragment.addObserver(key, function() {
        // trigger stuff
      });
    });
  },

But since it's an anti-pattern it can potentially lead to unexpected behaviours. Thoughts?

benoror avatar Jan 04 '17 06:01 benoror

@benoror I think you could also use defineProperty inside the fragment itself to create a single computed property with something like

init() {
  this._super();
  let keys = [];
  this.eachAttribute((key) => {
    keys.push(key);
  }
  keys.push(function() {
    // code here
  });

  // also works with addObserver
  Ember.defineProperty(this, 'propertyName', Ember.computed(keys))
}

jakesjews avatar Jan 23 '17 01:01 jakesjews

I'm seeing this issue too. I tried defineProperty, but it seems to be the same as using computed. Is there any way to get this to work?

RobbieTheWagner avatar Jul 14 '20 14:07 RobbieTheWagner