Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Observer on property in parentView.parentView not working #3693

Closed
ctimmerm opened this issue Nov 5, 2013 · 8 comments
Closed

Observer on property in parentView.parentView not working #3693

ctimmerm opened this issue Nov 5, 2013 · 8 comments
Assignees
Labels

Comments

@ctimmerm
Copy link
Contributor

ctimmerm commented Nov 5, 2013

Observing a property more than one level up in the parentView chain does not seem to work. This used to work in earlier release candidates.

App.View = Ember.ContainerView.extend({
  childViews: 'foo'.w(),

  foo: Ember.ContainerView.extend({
    childViews: 'bar'.w(),

    bar: Ember.ContainerView.extend({
      testDidChange: function() {         
        alert(this.get('parentView.parentView.test')); 
      }.observes('parentView.parentView.test').on('init')
    })
  })
});

jsbin

Ember.meta(App.view) shows that there are no chainWatchers and watching.test is undefined.

@wagenet
Copy link
Member

wagenet commented Nov 8, 2013

In general, these sorts of parentView chains are a sign of code smell. However, it seems like this should work. Can you put together a JSFiddle that demonstrates the issue?

@ctimmerm
Copy link
Contributor Author

ctimmerm commented Nov 9, 2013

I already had a jsbin attached, here's the jsfiddle: http://jsfiddle.net/66qux/.

This is the commit that introduced the regression: d4fd799
(Swapping https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/system/core_object.js#L132 with the line above makes the example work.)

I know this is not an idiomatic use of observers, but it's a pattern we often use to build complex reusable views (our app is basically a SproutCore 1.4 app ported to Ember using Flame).

To give an example, if I declare the following view:

App.ComplexView = Flame.View.extend({
  childViews: 'contentView'.w(),
  label: '',
  button: '',

  contentView: Flame.View.extend({
    childViews: 'labelView buttonView'.w()

    labelView: Flame.LabelView.extend({
      // Looks cumbersome and fragile, but in Flame we can achieve the
      // same using Flame.computed.nearest('label')
      value: Em.computed.alias('parentView.parentView.label')
    }),

    buttonView: Flame.ButtonView.extend({
      title: Em.computed.alias('parentView.parentView.button')
    })
  })
});

I can then use it without having to care about its complex structure:

App.ComplexView.create({
  label: 'foo',
  button: 'bar'
})

The way Ember currently works, changing the label or button properties would not invalidate the values cached by the computed property.

Debugging this I noticed that when the get in testDidChange is called on init, and finishChains starts the didChange chain, for the last ChainNode (the one with this._key === 'test') it does not add a chainWatcher on the object. Which is because for that node, this._parent.value() returns undefined, which in turn is because Ember.meta(this._parent._parent._value).proto === this._parent._parent._value making lazyGet return undefined.

@wagenet
Copy link
Member

wagenet commented Nov 9, 2013

Whoops. I missed the JSBin. Sorry!

@ghost ghost assigned krisselden Nov 9, 2013
@wagenet
Copy link
Member

wagenet commented Nov 9, 2013

@kselden This is related to your work. It still seems like it should work but maybe you will notice something I'm missing.

@wagenet
Copy link
Member

wagenet commented Apr 15, 2014

@krisselden ping.

@krisselden
Copy link
Contributor

@ctimmerm I did give feedback, I reviewed the jsbin, finishChains(this); in the parentView should be handling this case, but it isn't, it assumes that only the chains that could have been added are by the parentView itself, so it leaves the chains the childView setup unfinished, this is because the parentView is giving a un-fully-initialized this to createChildView which is installing a chain observer.

@krisselden
Copy link
Contributor

To further clarify, it should be finishing chains from the nodes in its meta.chainWatchers, not starting at meta.chains.

@krisselden
Copy link
Contributor

fixed in #5575

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants