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

this.$() returns undefined in didInsertElement hook with Ember 1.7.0 #5452

Closed
herom opened this issue Aug 20, 2014 · 22 comments
Closed

this.$() returns undefined in didInsertElement hook with Ember 1.7.0 #5452

herom opened this issue Aug 20, 2014 · 22 comments
Labels

Comments

@herom
Copy link

herom commented Aug 20, 2014

I have a view which has a method declared as follows:

addDynamicCss: function () {
  var $this = this.$(),
        dynamicCss = this.get('cssInfo');

  $this.removeAttr('style');
  $this.css(dynamicCss.properties);
}.observes('cssInfo').on('didInsertElement')

While this code works with Ember up to v1.6.1 without any problems, in v1.7.0 this.$() resolves to undefined and therefore gives me the following error:

TypeError: Cannot read property 'removeAttr' of undefined
@gpoitch
Copy link
Contributor

gpoitch commented Aug 20, 2014

@herom it can be undefined when your method gets invoked by that observer before the view has been inserted into the DOM. But on didInsertElement it is working correctly.

@rwjblue
Copy link
Member

rwjblue commented Aug 20, 2014

I agree with @gdub22, this is likely due to cssInfo being changed.

Closing for now, but I'm happy to reopen with a JSBin reproducing the error.

@rwjblue rwjblue closed this as completed Aug 20, 2014
@herom
Copy link
Author

herom commented Aug 20, 2014

Thanks a lot @gdub22, if I remove observes('cssInfo') this.$() is indeed what I expect. But what causes the different behaviour in v1.7.0?

In my case, I have to observe the cssInfo property to show/hide the view in the DOM. Until now, I always thought that this notation .observes('someProp').on('someHook') sets up the observer for the property right after the hook was fired (and until now, I experienced this behaviour as well). With v1.7.0 it seems that this is not respected anymore - am I right? So, does anyone have a simple solution to this specific case? Any help is much appreciated! 👍

@stefanpenner
Copy link
Member

@herom you should make a computed property for the style, and then use bindAttr or something to bind it to the attribute. You will never have this problem again.

Observers are really low level, and require you to very careful.

@knownasilya
Copy link
Contributor

@herom

cssInfo: function () {
  // do something dynamic to set value..
  return dynamicClassName;
}.property()
<div {{bind-attr class='view.cssInfo'}}>
  <!-- something here -->
</div>

@herom
Copy link
Author

herom commented Aug 20, 2014

@rwjblue I created 2 jsbin examples - only diverging by their used Ember versions.
Here (working) with Ember v1.6.1: http://emberjs.jsbin.com/fasolebomite/2/edit

and here (breaking) with Ember v1.7.0: http://emberjs.jsbin.com/qeyezihezaki/1/edit

I know that the example is somewhat silly, but it illustrates the issue 😄

@stefanpenner I'll look into your suggested solution. The problem will just be that my resolved cssInfo object will contain CSS classes as well as dynamic computed CSS properties, so I would have to use {{bind-attr}} twice, for class and style attribute... is this even possible?

@knownasilya
Copy link
Contributor

@herom should be able to do {{bind-attr class='' style=''}}

@knownasilya
Copy link
Contributor

this.$() seems to be returning undefined. Looks like a regression.

@stefanpenner
Copy link
Member

@knownasilya doesn't appear to be for me (when removing the observer)

@knownasilya
Copy link
Contributor

@stefanpenner so should it return the correct value when doing observes('..').on('didInsertElement') wouldn't that fire it just after the internal didInsertElement stuff runs? At least that's how it behaved pre 1.7.0, so was that incorrect behavior before, or is it incorrect now?

@gpoitch
Copy link
Contributor

gpoitch commented Aug 20, 2014

Seems to be due to the change in Ember.computed.alias (modified in 1.7.0)
Working by removing the alias: http://emberjs.jsbin.com/qeyezihezaki/3/edit

Reference: #5120

@knownasilya
Copy link
Contributor

@gdub22 👍

@herom
Copy link
Author

herom commented Aug 20, 2014

thanks for taking the time to look into the code @gdub22 but I always thought that Ember.computed.alias exists to prevent me from writing code like this:

cssInfo: function() {
    return this.get('controller.cssInfo');
  }.property('controller.cssInfo'),

so I can do just

cssInfo: Ember.computed.alias('controller.cssInfo')

to achieve the same with less boilerplate code?

This seems like a regression to me, as I would have to produce the above code to have a workaround to what I had already achieved by using Ember.computed.alias prior to v1.7.0. @rwjblue @stefanpenner - was/is this intended behaviour?

@gpoitch
Copy link
Contributor

gpoitch commented Aug 20, 2014

@herom yea that's the idea. Just did that to try and find the root cause

@stefanpenner
Copy link
Member

shit is AliasDescriptor in 1.7 ? I thought it wasn't landing till 1.8

#5289 is my WIP to fix it.. (although I don't know if ^^ is a bug or actually expected behaviour)

@stefanpenner
Copy link
Member

Im am fairly certain #5289 will address this. Bah i thought AliasDescriptor was going to be part of the 1.8 release, but I suppose bugfix beta means beta...

@stefanpenner stefanpenner reopened this Aug 20, 2014
@herom
Copy link
Author

herom commented Aug 20, 2014

thanks for reopening and explanation @stefanpenner - seems like I'll stay at v1.6.1 for a while...

@gdub22 thanks for investigation 👍

@stefanpenner
Copy link
Member

@herom my above suggestions of using bindings + cp's is still a much better solution then using an observer. Remember you want data to flow based on consumption not based on churn. Basically you want the template to pull the data from the rest of the system, not manually push data.

That being said, we can use this issue to track for now, because it does provide an example manifestation of the problem.

@herom
Copy link
Author

herom commented Aug 20, 2014

@stefanpenner I see. So if the suggested way is to use bindings and computed properties I'll try and see what I can do 😃

@stefanpenner
Copy link
Member

@herom correct, observers are pretty low level. CP's and bindings are typically the more user friendly way of doing things.

@wagenet wagenet added the bug label Aug 22, 2014
@krisselden
Copy link
Contributor

It is not the on('didInsertElement') hook that is firing in this case, it is the .observes('cssInfo'), and the goal of alias change is that should behave identical to observes('controller.cssInfo').

@herom you should not have observers in views that don't check that when the property changes that the view is actually in DOM, if the view does not have element, then $() returns undefined. Yes, we did change alias, the idea is that alias should mirror the behavior of its target property, before it had the same laziness as a CP, but this had the problem of if a CP was computed and you observed the alias, the alias wouldn't also be computed.

@stefanpenner the above behavior won't be fixed by #5289 because the target of the alias isn't lazy, the alias was adding laziness. The idea of #5289 is to ensure that get/set/observes has the identical behavior on the alias as the target.

This particular change is intentional. Before this change, it worked for you, but having observers in a view that update DOM and do not guard against it not being in DOM has always been a fragile thing to do.

@herom
Copy link
Author

herom commented Sep 8, 2014

@krisselden thanks for the explanation - in the meanwhile I came around this issue by wrapping my code in a Ember.run.scheduleOnce('afterRender') run loop which works pretty well in my case 👍

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

7 participants