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

additional callback arguments ignored #6

Open
sa-0001 opened this issue Nov 21, 2015 · 2 comments
Open

additional callback arguments ignored #6

sa-0001 opened this issue Nov 21, 2015 · 2 comments

Comments

@sa-0001
Copy link

sa-0001 commented Nov 21, 2015

Working example using 'sync':

request = require('request')
sync = require('sync')
sync () ->
  [req, res] = request.sync(request, {})

Working example using 'syncho':

request = require('request')
syncho = require('syncho')
syncho () ->
  req = request.sync(request, {})

It's clear from looking at Future.prototype.resolve that only the first callback argument is returned; all additional arguments are simply dropped. I'm not sure whether this is a conscious decision but it means all async functions with more than one result need to be wrapped to be able to use them with syncho.

I added the following to the top of Future.prototype.resolve so that Function.prototype.sync returns the same way sync does, but there could be more places I'm missing:

var res = Array.prototype.slice.call(arguments, 1);
if (res.length === 0) {
  res = undefined;
} else if (res.length === 1) {
  res = res[0];
}

Is this an intentional design decision? If so, is there a chance of adding a config flag to make syncho behave like sync? I would be happy to make a pull request if you would let me know your thoughts on this.

@jtblin
Copy link
Owner

jtblin commented Nov 22, 2015

@twykke yes it was a conscious decision. There are not a lot of libraries that return more than one result as it isn't really idiomatic, request being one of them returning the body as a 2nd result. I agree that it can be a bit annoying. Changing it now would be a breaking change, however I am not sure it's worth having a config flag, maybe better just bumping the major version. I'll accept a PR if you want to do it either way.

Out of curiosity, there are generators now in node core which basically provide the same functionality than fibers, and async/await readily available in es7 using babel, so why are you still using fibers? Stuck in < 0.12 version?

@sa-0001
Copy link
Author

sa-0001 commented Nov 22, 2015

@jtblin thank you for responding. i understand your decision; actually in my opinion both syncho and sync have their issues, neither of which is easy to solve (syncho drops additional arguments; sync returns an array only when there is more than one callback result, but the number of results can vary). it seems like neither of us is certain what the 'best' behavior is.

in any case, at the moment one can simply fork and add a few lines of code to have it work the same way as sync.

i have been using sync for years now and use several major sync wrappers classes (mongo, mysql, pgsql, redis, ...) as well as all sort of utility functions. it's pretty much sync all the way down the stack. i have looked at libraries such as co which have wrappers for almost everything and would almost certainly achieve the same result, but it's still not just a matter of switching out 'sync/syncho' for 'co'. also, it's mostly coffeescript, which only added support for generators earlier this year.

EDIT/unrelatedNote: I created an issue for sync (link to issue) which I later discovered also affects syncho. So, either I could create this issue for syncho as well because the problem exists in both modules, or since they are both affected, it could be something common to fibers and unsolvable?

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

No branches or pull requests

2 participants