GeppettoJS / backbone.geppetto

Bring your Backbone applications to life with an event-driven Command framework.
http://geppettojs.github.com/backbone.geppetto/
MIT License
203 stars 28 forks source link

Add contextEvent mapping for all wirings (except values) #57

Closed creynders closed 10 years ago

creynders commented 10 years ago

As discussed in #50 I'd like to propose to add contextEvent mapping for all wirings.

Two caveats:

  1. I didn't add automatic context event mapping for values. It would be possible, but there's a problem with value wiring I'll be describing in 58 and thus left it out. Depending on how 58 goes I can still add it. but since dependencies aren't resolved for values it seems to me that contextEvents shouldn't be mapped either.
  2. The prototypes subject to contextEvent mapping need to extend Backbone.Events (or at least fulfil validateListen)

And some food for thought: an instance is responsible for removing these context event listeners itself. This is something we definitely need to point out. Or find a workaround for. ATM, i.e. in the current code base, this is a problem for all views bound with bindContext as well.

creynders commented 10 years ago

On a side note: could we get a patch release soon? (I don't think minor's necessary?) If so, I'd like to squeeze this PR in too.

geekdave commented 10 years ago

but since dependencies aren't resolved for values it seems to me that contextEvents shouldn't be mapped either.

Agreed.

The prototypes subject to contextEvent mapping need to extend Backbone.Events

What if someone forgets to do this? Would their contextEvent map just silently be ignored? Or would they hit an error? In either case, I'd prefer to either fail fast with some descriptive error, or (perhaps) automatically mix-in Backbone.Events into the object.

could we get a patch release soon

Absolutely. To give me a jump start, would you mind updating the README to reflect your recent changes?

geekdave commented 10 years ago

an instance is responsible for removing these context event listeners itself.

This has always been the case with using the programmatic listen() method as well, right? There shouldn't be any new risk of listeners hanging around too long as a result of opening this up to the declarative contextEvents right?

creynders commented 10 years ago

What if someone forgets to do this? Would their contextEvent map just silently be ignored? Or would they hit an error?

Hard error at configuration time. I'll just reuse validateListen I suppose.

This has always been the case with using the programmatic listen() method as well, right?

Yes

There shouldn't be any new risk of listeners hanging around too long as a result of opening this up to the declarative contextEvents right?

No, no new risk.

To give me a jump start, would you mind updating the README to reflect your recent changes?

Sure, will do!

creynders commented 10 years ago

Ah, a hard error at wiring time is not possible, but one does get thrown at resolution time. I.e. this is ready for merge, except for the docs.

BTW: I'll look into the grunt task sequence, since it's really annoying the minified file always gives merge conflicts whenever multiple PR's live at the same time. We should be able to test w/o running the minification.

geekdave commented 10 years ago

one does get thrown at resolution time

Sounds good to me.

the minified file always gives merge conflicts whenever multiple PR's live at the same time

Yeah that is really annoying. What are other projects doing? Probably the best would be some kind of post-merge build that minifies & publishes to a separate gh-pages branch (which can still be linked from the README)

geekdave commented 10 years ago

Looks like there is a merge conflict in createChildResolver - it wasn't immediately clear to me how to resolve this, so I'll leave it to you, @creynders .

Let me know if you're ready for a new release after this PR.

creynders commented 10 years ago

:shipit: