twitter/flight — pull request #68 by kevinrobinson (2013-03-11 03:43:38+00:00)
Title: Relaxing Utils#push protections to allow components to override #defaultAttrs already set in a mixin

Really interesting project, thanks for sharing!

I was reading through the demo app, and noticed that in the `with_select` mixin, there are a few attributes that it uses as part of its core functionality, including the name of the class to apply to selected elements (`selected`) and the name of the event to fire when selection changes (`selectionChangedEvent`). These are expected to be defined on the component at event time when they need to be used by the mixin.

This is clear reading through the source, but I was expecting to see them defined in the call to `defaultAttrs.` I added them in and was surprised to find that this won't work if other components consuming the mixin also define defaults for those attributes (e.g., `mail_items`), since Component#defaultAttrs is strict when merging those attributes in.

I see the desire to want to make option clobbering throw an exception, but it seems like a common pattern is to have a mixin define a default attribute and then have a consuming component override it. Since the ordering is reversed in the mixing-in process (i.e., component first, then mixin), it effectively means that if mixins have defaults it prevents consuming components from overriding those defaults.

So I patched the Utils#push method to change the meaning of the `protect` argument, since it didn't look like it was being used in any other code. Instead of throwing an exception, the `base` object just wins when there is a conflict between it and the `extra`. This makes sense here since it's the #defaultsAttr method calling it - you don't need the default attribute value if one is already set, so no harm done.

Another benefit of this would be that mixins could now use #defaultAttrs to declare what attributes they expected to be provided by mix-in time. I also update the demo app to use this (which I'll submit in a separate PR).

Curious to hear what you think, or if there are other side effects I'm inadvertently introducing here. Thanks!