search

emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.47k stars 4.21k forks source link

classNameBindings don't get merged into classNames for tagless component #9417

Closed meirish closed 10 years ago

meirish commented 10 years ago

http://emberjs.jsbin.com/zamakubudi/11/edit?html,js,output

The linked bin works in 1.7, and doesn't work in 1.8 (specifically the classes on the div). I believe it was working as recently as ember 1.8-beta4.

I realize this isn't a common use case and have changed our app accordingly, but thought I should file this anyway as @mixonic said in #emberjs-dev that it's most likely a bug.

twokul commented 10 years ago

@mixonic did I break it? I will take a look into it

mixonic commented 10 years ago

@twokul it is a weird use case, but I'm unsure why it fails. All bindings used to be merged on to classNames, and now they don't seem to be. I'd appreciate you looking into it. I think it is a bug and significant enough of a divergence of behavior to violate semver, but I'm not sure. Would like your thoughts after some research!

twokul commented 10 years ago

@mixonic This check avoids parsing classNameBindings if tagName is an empty string. Since tagName is set to an empty string, IMO it doesn't make sense to parse classNameBindings because you don't have a tag to attach classes to.

Maybe, we should warn developers when classNameBindings is specified and tagName is an empty string to help them to avoid confusion and bugs?

Violating semver is obviously bad but I would be hesitant to bring 1.7 behaviour back.

mixonic commented 10 years ago

@twokul I agree an assertion would be enough :-D If you can whip something up we can get it into master and into what I expect will be a 1.8.1 release in the next week or two.

@meirish we're going to call this a correction of a bug, and not a new bug itself. I think @twokul's logic makes sense here, and the performance argument is real. classNameBindings and friends are supposed to spec the classes of that view's element. If the view doesn't have an element, there isn't a reason to process them. I suggest writing a custom CP for your use case, instead of of relying on the old behavior.

meirish commented 10 years ago

@mixonic & @twokul thanks, I agree that this makes sense. thanks for all of your hard work as well!

lukemelia commented 10 years ago

Since the beginning of Ember time, the presence of classNames or classNameBindings was enough to make a view render as a div instead of a metamorph. I'm OK with this as a performance improvement, but I think it would be good to assert in development if a view has classNames and/or classNameBindings defined and an empty tagName.

mixonic commented 10 years ago

@lukemelia if a tagName is "" and classNameBindings exist then there should certainly be an assert, right? This means we have a confusion of intent.

If you apply classNameBindings to an Ember.MetamorphView they no-op in Ember 1.7. I'm not sure where else you would apply them but possibly not have a div already.

stefanpenner commented 10 years ago

+1 throw an exception.