Skip to content

Added a method to attach "Node-style" callbacks as handlers.#150

Merged
briancavalier merged 1 commit intocujojs:devfrom
Schoonology:dev
Jun 10, 2013
Merged

Added a method to attach "Node-style" callbacks as handlers.#150
briancavalier merged 1 commit intocujojs:devfrom
Schoonology:dev

Conversation

@Schoonology
Copy link
Copy Markdown
Contributor

TL;DR: This makes integrating with callback users easier.

See #149 for more discussion.

@Schoonology
Copy link
Copy Markdown
Contributor Author

Ready for review @briancavalier .

@briancavalier
Copy link
Copy Markdown
Member

Awesome, thanks. Stinks that travis still seems busted :( I should have time tomorrow (EST) to review.

@briancavalier
Copy link
Copy Markdown
Member

Sorry it took me a while to get back to this. For some reason, github didn't notify me when you updated the PR!

This makes integrating with callback users easier.
@Schoonology
Copy link
Copy Markdown
Contributor Author

@briancavalier Updated.

@Schoonology
Copy link
Copy Markdown
Contributor Author

For posterity, more discussion can be found in these previous revisions:

@briancavalier
Copy link
Copy Markdown
Member

I pulled and ran the unit tests locally, since Travis still seems busted. Looks great. Thanks for proposing this and working through the details!

briancavalier added a commit that referenced this pull request Jun 10, 2013
Added a method to attach "Node-style" callbacks as handlers.
@briancavalier briancavalier merged commit f341944 into cujojs:dev Jun 10, 2013
@briancavalier
Copy link
Copy Markdown
Member

Hey @Schoonology, I'm wondering if you've been using liftCallback and bindCallback via the dev branch? If so, do you have any thoughts about whether bindCallback(promise, callback) or bindCallback(callback, promise) might be a "better" param ordering based on your experience so far? I'd def appreciate hearing your latest perspective, as I'm getting ready to release this stuff within the next week.

Thanks!

@Schoonology
Copy link
Copy Markdown
Contributor Author

I have been, and I still like bindCallback(promise, callback). I realize that not only is "callback is the ultimate parameter" a convention, but the promise is really the subject of bindCallback, implying it should be the primary argument.

Does that make sense? Thoughts?

@briancavalier
Copy link
Copy Markdown
Member

Yep, that does make sense. It may also depend on the usage scenario. Going back to your original message, it seems like one primary use case is creating "combo" APIs that both accept a nodeback and return a promise. Like you said, since the nodeback is by convention the last param, it makes sense to provide a parallel to that.

It also means that liftCallback and bindCallback provide coverage for both argument orderings, which is certainly useful. We'll go with it for 2.2.0.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants