Adding option to global and local control for adjustCallback #39

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants

wookieb commented Jul 1, 2012

Before commit

pool.Pool({
 name: 'test',
 create: function() { callback({some:'value'}) }
});
pool.acquire(function(client) {
  // passing client/result object as first argument of callback 
  // may confuse flow control modules like node-seq and others
  console.log(client.some); // value
});

After commit adjustCallback behavior can be controlled.


// local configuration
pool.Pool({
 create: function() { callback({some:'value'}) },
 adjustCallback: false,
 name: 'test'
});
pool.acquire(function(){
  var error = arguments[0], client = arguments[1];
  console.log(error); // undefined
  console.log(client.some); // value
});

// global configuration
pool.Pool.adjustCallback(false);
pool.Pool({
 name: 'test',
 create: function() { callback({some:'value'}) }
});
pool.acquire(function(){
  var error = arguments[0], client = arguments[1];
  console.log(error); // undefined
  console.log(client.some); // value
});

// global flag can be overwritten by local option
pool.Pool.adjustCallback(false);
pool.Pool({
 name: 'test',
 create: function() { callback({some:'value'}) },
 adjustCallback: true
});
pool.acquire(function(client){
  console.log(client.some); // value
});

wookieb commented Jul 1, 2012

Fix #32

Owner

coopernurse commented Jul 2, 2012

Hi there,

Thanks for the pull request. I wonder if it would be easier to simply declare that factory.create() functions must invoke callbacks using two arguments: (error, resource) and eliminate the adjustCallback code completely.

The adjustCallback() mechanism was an attempt to preserve backwards compatibility, but at this point just creates confusion. Given that virtually all node.js libraries rely on error being the first callback argument, I think most folks are using that in the create functions.

Since it's a non-backwards compatible change, I'd suggest we cut a new 1.0.x release that simply logs a warning if the create function doesn't have two arguments, and then cut a 1.1.0 release that removes adjustCallback.

I think that would make this pull request unnecessary, and would fix #32.

What do you think?

-- James

wookieb commented Jul 2, 2012

I'am totally agree with you.
I just want to create solution non-invasive to everyone. I'll remove adjustCallback when I back home.

wookieb commented Jul 5, 2012

Fixed :)

Owner

coopernurse commented Jul 7, 2012

Thanks! I'm still on vacation, but when I get home I'll work on getting the new release done. I will mark it 2.0.0 since this is not backwards compatible. There is at least one other non-backwards compatible change I want to make to clean up the API a bit.

coopernurse added a commit that referenced this pull request Jul 23, 2012

#39 and #32 - remove adjustCallback and require acquire callback to …
…accept (err, obj) - not backwards compatible.
Owner

coopernurse commented Jul 23, 2012

Ok, I just committed a patch that removes adjustCallback() -- I haven't pushed this to npm yet. If you have time, please grab this change from master and try it in your app and let me know if it works. If so I'll release this as 2.0.0 (to indicate that the change is not backwards compatible).

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