Skip to content

IcedCoffeeScript callbacks incompatible with support for legacy single-argument callbacks to acquire #32

Closed
senotrusov opened this Issue Feb 11, 2012 · 5 comments

3 participants

@senotrusov

Arity checks for acquire() callback (added with commit a3b65a371e) are incompatible with IcedCoffeeScript callbacks.

IcedCoffeeScript callbacks internally use arguments[] and does not reveal the exact number of arguments via .length property.

I suggest to not check arity, at least by default.

@wookieb
wookieb commented Jul 1, 2012

It is incompatible with flow control libraries like "node-seq". Simply it recognizes returned object as error.
In my opinion we (as programmers) should have an option to turn off that behavior.

@coopernurse coopernurse added a commit that referenced this issue Jul 23, 2012
@coopernurse #39 and #32 - remove adjustCallback and require acquire callback to a…
…ccept (err, obj) - not backwards compatible.
3292365
@coopernurse
Owner

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).

@wookieb
wookieb commented Jul 25, 2012

Sorry for my "lag" in response but I was really busy.
BTW Of course it works for me! :)
But I found a slightly strange case. Function "createResource" attempt to make something "similar" to "adjustCallback".

    var Pool = require('generic-pool');
    var p = Pool.Pool({
            name: "mysql",
            create: function (callback) {
                    var o = {'test':1};
                    callback(o);
            },
            destroy: function (client) {
            },
            max: 20,
            idleTimeoutMillis: 100
    });

    p.acquire(function(error) {
            console.log(arguments); // 0 = null, 1 - my test object
    });

Are You really sure that is correct and expected (for everyone) behavior?

@wookieb
wookieb commented Jul 26, 2012

After longer reflection I think the above behavior is correct :)

@coopernurse
Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.