sproutcore / sproutcore

JavaScript Application Framework - JS library only
sproutcore.com
Other
2.15k stars 291 forks source link

'@each' doesn’t Work in Conveniently Created Observers #986

Open OliFFM opened 11 years ago

OliFFM commented 11 years ago

Observers created with the convenient method seem to not work:

eachTitleObserver: function()
{
    console.log('ObserverTest.listController.eachTitleObserver');
}.observes('@each.title')

Changing any property like

ObserverTest.listController.get('content').firstObject().set('title', 'New Title')

is not recognised by the observer.

Workarounds: (1) Create a normal function and change it to an observer, e.g. in init():

ObserverTest.listController = SC.ArrayController.create(
{
  init: function()
  {
    sc_super();
    this.addObserver('@each.title', this, 'eachTitleObserverCreatedInInit');
  },
  eachTitleObserverCreatedInInit: function()
  {
    console.log('ObserverTest.listController.eachTitleObserverCreatedInInit');
  }
});

(2) Observe a observing property:

ObserverTest.listController = SC.ArrayController.create(
{
  propertyObserver: function()
  {
    console.log('ObserverTest.listController.propertyObserver');
  }.observes('eachTitleProp'),

  eachTitleProp: function()
  {
    console.log('ObserverTest.listController.eachTitleProp');
    return this.get('content');
 }.property('@each.title').cacheable()
});
dcporter commented 11 years ago

Confirmed that the convenience method doesn't seem to work with "@each". I tried a number of different syntaxes (prepending with a period, etc.) and no dice.

publickeating commented 11 years ago

The @each property path has to follow the enumerable property it is referring to.

For example, .observes('arrangedObjects.@each.title') should yield better results.

Also, you can only use property paths for bindings and observers, not for computed property dependent keys. Computed property dependent keys can only be single property names.

For example, .property('otherProperty', 'anotherProperty') is the only valid syntax.

Sorry I didn't notice this earlier, I hope you figured it out already.

publickeating commented 11 years ago

… corrected the above to be .observes('arrangedObjects.@each.title') (added the '.' before @each)

Another example,

arrayObject.addObserver('@each.title', function () { … })
OliFFM commented 10 years ago

Before I reopen this bug I would like to ask if I don’t get it right, or if it is really a bug: @each still does not work if created with the convenient method (SC 1.10.1). E.g. taking the code of the book “SproutCore Web Application Development” and adding two observing methods to Contacts.groupsController, one within init, one is created with .observes(). Code looks like this:

Contacts.groupsController = SC.ArrayController.create(
/** @scope Contacts.groupsController.prototype */
{
  init: function() // New code
  {
    this.addObserver('@each.name', function()
    {
      console.log('init Observer works');
    })
  },

  nameObserver: function() // New code
  {
    console.log('This observer does not work');
  }.observes('arrangedObjects.@each.name'),

  removeGroup: function (sender) // Code from the book
  {
    var selection = this.get('selection');
    selection.forEach(function(group) { group.destroy(); });
  }
});

The anonymous observer method created within init fires as expected, but the observer method nameObserver doesn’t. I’ve tried

Am I doing something wrong, or is it worth to reopen the bug?

dcporter commented 10 years ago

I'm seeing this too. Reopened.

dcporter commented 10 years ago

Wanted to update this issue with @ktornwall's observations here that this appears to be an issue with chained observers. In the convenience method, '@each' is treated as a non-chained observer and ... kind of dies I guess? So it only observes changes in membership, ignoring each member's name property. If you use .addObserver, I gather that it automatically assumes chained observing, so you end up where you want to be. @ktornwall suggests that your convenience properties will work correctly if you add a * to trigger the chained observer behavior, so for example .observes('*@each.name'). Could someone verify?

If this is the case, I would vote that we do some under-the-hoodery to have @each always be chained. That seems like the clear intention to me. Thoughts?

OliFFM commented 10 years ago

Just tested: That really seems to work :-)

Am 10.02.2014 um 18:37 schrieb Dave Porter notifications@github.com:

Wanted to update this issue with @ktornwall's observations here that this appears to be an issue with chained observers. In the convenience method, '@each' is treated as a non-chained observer and ... kind of dies I guess? So it only observes changes in membership, ignoring each member's name property. If you use .addObserver, I gather that it automatically assumes chained observing, so you end up where you want to be. @ktornwall suggests that your convenience properties will work correctly if you add a * to trigger the chained observer behavior, so for example .observes('*@each.name'). Could someone verify?

If this is the case, I would vote that we do some under-the-hoodery to have @each always be chained. That seems like the clear intention to me. Thoughts?

— Reply to this email directly or view it on GitHub.

dcporter commented 10 years ago

Awesome, thanks @OliFFM!

So in the case of .@each.name the developer intent is crystal clear, and I think we should put in the effort under the hood to conform to it. Thoughts anyone?

katzenbar commented 10 years ago

I would say that manipulating behind the scenes is probably the best way to handle it for now. There are performance implications with using @each (since it needs to make so many observers and will fire frequently), but since there is no way around this we should make it as easy as possible to use. I would also like to see a "guide" article that tells you what it does, how to use it, and discusses the performance issues you may see.

I've been playing with angular lately and I have really liked the developer guide (not a tutorial) aspect of their documentation http://docs.angularjs.org/guide/ and I think SC could benefit from doing something similar with large core concepts, as well as tricky features like this.

On Mon, Feb 10, 2014 at 12:53 PM, Dave Porter notifications@github.comwrote:

Awesome, thanks @OliFFM https://github.com/OliFFM!

So in the case of '.@each.name' the developer intent is crystal clear, and I think we should put in the effort under the hood to conform to it. Thoughts anyone?

Reply to this email directly or view it on GitHubhttps://github.com/sproutcore/sproutcore/issues/986#issuecomment-34660500 .

OliFFM commented 10 years ago

I would vote to let as is, but document it. @each is an expensive operation, needing a star for using it (.observes('*@each.name')) makes this clearer. Additionally, in my opinion it is more consistent. But I also would accept manipulations behind the scenes ;-)

Off Topic: @ktornwall Yes, docs would be fine, there is a lot missing for SproutCore, but someone needs to write them. I myself have the problem that all kind of documentation that goes behind trivial usage of SproutCore is missing. Some of it could be found in blogs or discussions. Some very good docs that are currently missing can be found on the obsolete old SproutCore Wiki, but then you have to deal with the problem that these docs are for old SproutCore versions. I’m in the middle of the painful process of creating custom views that do more than basic stuff. I have big performance problems. And I have absolut no idea if all that assumptions that I’ve made are right, but I can’t verify because of missing docs – and large portions of the SproutCore code are difficult to understand if you are not a hardcore JavaScript coder. But to improve the situation I would need to write some docs by myself – which I didn’t because of having no time for that so far :-( Hence, I’m not in the role to say that others should do that …

publickeating commented 9 years ago

Worked on fixing this, but it's really difficult to reconcile all the special cases needed for @each. I'll need a lot more time to be able to get it working completely. So far, I've got it to work, but am seeing excess observers being added when more than a single .observes is used.

Work is in this branch: https://github.com/sproutcore/sproutcore/tree/team/publickeating/fixes-for-observes-and-each