Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Cannot observe changes to nested properties on array using @each #541

Closed
robharper opened this Issue · 63 comments
@robharper

Adding an observer on a nested property of objects within a collection does not reliably fire. I've created a test case that could be added to ember-runtime/tests/mixins/array_test.js that illustrates this. Observing a nested property on the EachProxy itself appears to work fine. However, observing the property using @each.nest.isDone does not.

In the following test, count1 is incremented to 1, count2 is not:

test('modifying nested contents of an object in an array should call @each observers', function() {
  var ary = new TestArray([
      Em.Object.create({ nest: Em.Object.create({ isDone: false}) }),
      Em.Object.create({ nest: Em.Object.create({ isDone: false}) }),
      Em.Object.create({ nest: Em.Object.create({ isDone: false}) }),
      Em.Object.create({ nest: Em.Object.create({ isDone: false}) })
    ]);

  var get = Ember.get, set = Ember.set;
  var count1 = 0, count2 = 0;

  // Works
  var each = get(ary, '@each');
  Ember.addObserver(each, 'nest.isDone', function() { count1++; });

  // Doesn't work
  Ember.addObserver(ary, '@each.nest.isDone', function() { count2++; });

  count1 = count2 = 0;
  var item = ary.objectAt(2);
  get(item, 'nest').set('isDone', true);

  equal(count1, 1, '@each.nest.isDone should have notified - observing @each array');
  equal(count2, 1, '@each.nest.isDone should have notified - observing chain containing @each');
});
@wagenet
Owner

@each is only supported one level deep. It would be nice to allow more, but I don't know how difficult this would be technically. I'll let @wycats or @tomdale chime in on this.

@wycats
Owner

I don't think that's actually true... It uses the normal chain infrastructure and should work arbitrarily many levels deep.

@wagenet
Owner

@wycats This sounds like a bug then. I've never seen it work more than one level deep.

@tomdale
Owner

Agreed, this sounds like a bug. I believe this was working at some point. Thank you for the unit test.

@endash

So I've been puzzling through this. It seems to boil down to EachProxy handling setting up the observers internally when you call addObserver with itself as the root. When as part of a chained path, however, the ChainNode plucks the top-level keyname off of the EachProxy, and goes on its merry way, and the EachProxy never gets involved. Basically, there are two levels of arrays at play.

watching '@each.nest.@each.isDone' DOES work, in this instance.

@endash

Forgot to clarify that when the ChainNode plucks the keyname off the EachProxy, it's getting an EachArray, hence the need for nesting the @ each observers.

@krisselden
Owner

The way chains work is each chain node has a corresponding chainWatcher that watches the key on the node's parent's value(), in order for this to work the chain needs to stop at @each and setup the chainWatcher with the remaining path.

I'm considering special casing @ character to just mean this.

@wagenet
Owner

@kselden, does your simplify-properties branch touch on this at all?

@wagenet
Owner

Looks like this is still an issue.

@krisselden
Owner

chains do not chain through properties that produce arrays, this requires some changes I stated above, no I haven't done any work on it. It also is problematic @each itself changes when the array does.

@tpitale

I've hacked around this, for anyone that is facing this issue (especially when using ember-data associations), by adding a property on one side of the association that checks the property in the other end of the association (in my case, isLoaded) and then using that after the @each. As I said, this is a hack. But until 1.1, it works for me.

@drogus

@tpitale: could you show an example of such hack? I'm not sure what you mean by adding a property on other side of association.

@kselden @wagenet: I encountered something similar recently, but frankly I'm not sure if this is the same bug, could you take a look at this fiddle: http://jsfiddle.net/uErrd/4/ ?

@tpitale

@drogus given the original example, there would be a nestIsDone property for the path nest.isDone. So you could observe @each.nestIsDone.

@mspisars

would really love to see this in 1.0 not 1.1 :+1:

@wagenet
Owner

@mspisars Me too, but I don't want to block 1.0 on things that are difficult to fix and non-essential.

@darthdeus

last activity 4 months ago ... :+1:

@wagenet
Owner

@mspisars, @darthdeus I would love to see it in 1.0 too. The issue is not about desire, it's about ability.

@phammer

:+1: for 1.0: would be really useful to have this working.

@pdwinkel

:+1: for 1.0

@wagenet
Owner

Public Service Announcement: +1s do not provide development resources where they are lacking.

We've already agreed that we'd love to have this feature but it's very much non-trivial to support. It's certainly not something we'll block 1.0 on.

@endash

Agreed with @wagenet these are non-trivial code paths that touch mission critical parts of ember.

@HWest

Any updates on this? We have been trying to use this for a web app that's about to be used by a major Hotel chain and none of the workarounds seem to work. The only workable solution at this point is to redesign this part of the interface to make it simpler which is a shame.
Any information would be greatly appreciated and thanks for all your great work.

@joliss

It's a surprisingly hard problem. Part of the issue is that even if you implemented it or worked around it, updating a dependent array (which seems to be be the most common use case) would take O(n*2) time, which is a big problem even for fairly small datasets. To get it to be O(n), we'd need smarter array primitives to compose dependent arrays. There has been *some work on this by Peter and others, and people are definitely interested in pushing it forward, but I wouldn't hold my breath.

Related: #1288, #2711.

For now I'd recommend generating the dependent arrays on the server, basically as "computed relationships".

@tim-evans

Here's another fiddle demonstrating this issue: http://jsfiddle.net/3bGN4/268.

@meelash

Example workaround, demonstrating nested @each:

https://gist.github.com/meelash/5785328

@dandehavilland

If this is on the backburner, it would be helpful to note this behaviour in the API docs (maybe I missed it?) as it took me a while to find this ticket.

@luxferresum

@dandehavilland Exactly! It should definitly be noted here: 'http://emberjs.com/guides/object-model/computed-properties-and-aggregate-data/'. "@each.goo.bar" is still not working, i've needed days to find this out.

@kingpin2k

But technically you can view nested properties, just not nested collections of properties, right?

works - flagpole and flag are singular

    schools.@each.flagpole.@each.flag

but should be

    schools.@each.flagpole.flag

** doesn't work - flags is a collection**

    schools.@each.flags.@each.flag 
@joliss

@kingpin2k No, I don't believe any of those work. They won't update correctly.

[Edit: maybe I'm wrong]

@wagenet
Owner

@kselden is this something you're likely to work on any time soon?

@wagenet
Owner

This is documented as not working. It would be great to add, but it's obviously difficult / not high enough a priority. If someone wants to work on it, we'd be happy to review a PR.

@wagenet wagenet closed this
@hbarrington hbarrington referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@SergeySolonko

You can just add new computed property, which will contains only very nested object, which you're trying to observe. And @each will works correctly
Use next method

  allTickets: function () {
    var sections = this.get('sections'),
      each = Ember.$.each,
      tickets = [];

    each(sections, function (i, section) {
      each(section.rows, function (j, row) {
        each(row.tickets, function (k, ticket) {
          tickets.addObject(ticket);
        })
      });
    });

    return tickets;
  }.property(),

  checkedCount: function () {
    ///custom logic
    return something;
  }.property('allTickets.@each.is_checked')

instead of

  checkedCount: function () {
    ///custom logic
    return something
  }.property('sections.@each.rows.@each.tickets.@each.is_checked')
@panayi

This is so easy to miss in the guides. I believe it should issue a warning when a nested @each is found.

@dowsanjack

Yea, I agree. It should at-least throw a warning :+1:

@m-obrien m-obrien referenced this issue from a commit in emberjs/website
@joliss joliss Document that nested @each does not work acb945c
@rwwagner90

This is still an issue many years later, correct? Is there a supported workaround of some sort?

@kingpin2k

Yes, you could always just build up computed properties at each level and watch the computed property instead of each property on each collection.

@rwwagner90

@kingpin2k I tried something like that, and it didn't work. Had to use .observes because the property never evaluated itself. There should really be some sort of built in Ember stuff to handle this, in my opinion.

@kingpin2k

Here's an example: http://emberjs.jsbin.com/faximekico/2/edit You essentially need to aggregate the values at the mid level.

@rwwagner90

I understand how to do it, the properties just don't fire, so observes had to be used.

@kingpin2k

Can you show me an example, properties are lazily loaded, so if you aren't actually attempting to get the properties, they won't "fire".

@rwwagner90

I never did an explicit .get to retrieve the property in question, I did have another computed property that was supposed to listen to it though. It seems that only an explicit get or use in a template makes them fire though, and not having another property observe them.

@kingpin2k

Correct, properties are only calculated on demand, just watching a property doesn't mean the property should be calculated. Rightfully so, if no one wants to use the property, there is no need to calculate the property.

@rwwagner90

@kingpin2k so what is recommend in this case? We're not supposed to use observes anymore.

@kingpin2k

My example shows how to accomplish the nested each dependencies without an observer. I don't recall anyone saying observers shouldn't be used anymore, but I don't religiously pay attention.

@rwwagner90

@kingpin2k your example uses a property, .property('model.books.@each.titles')

That gives me the same problem where it won't fire, as it is a property. I have another property that needs to observe that property.

@alexspeller

@rwwagner90 the approach definitely works, I suspect there is another issue in your code causing this problem. Try posting a jsbin showing the problem to figure out what it is

@kingpin2k

I might understand what you are getting at, could you modify my example to clarify though?

@alexspeller

@rwwagner90 regarding observers, my take on it is that using observers in cases when you're not interacting with non-ember code is a big code smell. So using observers to e.g. trigger a d3 graph rendering when your app data changes is totally legit, but using observers to set one ember property when another changes is almost always more easily accomplished with a different approach

@rwwagner90

@alexspeller @kingpin2k I have two properties, and one must change when the other changes. This is not possible though because properties don't evaluate, so the second property never updates.

@alexspeller

@rwwagner90 having one property that changes when another changes is a core feature of computed properties, so I'm not sure what you're describing - if you show the code you're using it's likely that someone can help.

@rwwagner90

@alexspeller I converted it all to using observes, but let me attempt to put it back and show you.

let extendedObject = Ember.Object.extend({
          valueLength: function() {
            return this.get('value').length;
          }.property('value.@each')
        });

Then I have a property that should update when valueLength does:

}.property('model.data.@each', 'model.data.@each.valueLength'),

However, this never fires, so I was forced to change my valueLength property to an observer that sets valueLength instead like so:

let extendedObject = Ember.Object.extend({
          valueLengthUpdater: function() {
            this.set('valueLength', this.get('value').length);
          }.observes('value.@each')
        });
@krisselden
Owner

@rwwagner90 your dependency keys don't match what you are getting, your dependency key should be model.data.length

@krisselden
Owner

Also, model.data.@each.valueLength is ok, by you should then actually get('valueLength') when computing a value. In a CP, generally speaking, if you are doing ending a dependency with just '@each' you aren't going to have a good time, and dependencies need to be consumed as part of the CP calculation. You cannot assume things about stuff changing together (stuff is invalidated synchronously and if there are observers that consume the property it can lead to race conditions), dependencies should align with what you actually consume as part of making the calculated value.

Generally, observers are low level, and hard to get right, you shouldn't actually do work in them (it can lead to race conditions, overly frequent recalculations, etc) they should only schedule work once per event frame.

@kingpin2k

Ditto @krisselden. Also, I'm assuming your model on some controller has a property data which is a collection of extendedObject and in the controller you have the computed property, and your extendedObject has a length property on it (which sounds like it should be an array, and not an object, but ignoring that:

Controller
something: function(){
   // inside of here you should actually fetch `valueLength` off of each item, or else it isn't really a dependecy
  // assuming data returns an array
  var lengths = this.get('model.data').getEach('valueLength');
}.property('model.data.length', 'model.data.@each.valueLength'),
@alexspeller

Edit:

You can replace valueLength property with valueLength: Ember.computed.alias('value.length').

Your dependent key then becomes:

}.property('model.data.@each.valueLength')

See @krisselden's explanation below on why what I wrote previously won't work.

@rwwagner90 valueLength is not necessary there - you can just use the value.length property. The property's dependent key can be simplified to:

}.property('model.data.@each.value.length')

@each shouldn't be used without a further property on the end. If you just want to observe when items are added or removed use somearray.[] instead. If you observe foo.@each.something then you don't need to also observer foo.[]

@krisselden
Owner

@alexspeller Actually you can't

@krisselden
Owner

@alexspeller get('@each.value') unfortunately returns an array of the values instead of returning an EachProxy which kills further chaining. I don't know why that was done that way but I don't know if I can change it given semver, and to be honest, I'm nervous about enabling this.

@kingpin2k

The squeaky wheel gets the oil, until it's affecting a larger portion of the ember community it won't get prioritized up, there is plenty of other low hanging fruit that helps 90% of the community.

@rwwagner90

@alexspeller Ember.computed.alias worked. Thanks! :+1:

@rwwagner90

@krisselden Chaining @each would be awesome, but no one has tackled it in the past few years, so it doesn't seem likely that it ever will be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.