WebReflection / dblite

sqlite for node.js without gyp problems
MIT License
209 stars 34 forks source link

[cadence] handle cadence callback signature #41

Closed mrose17 closed 9 years ago

mrose17 commented 9 years ago

hi there! when using the https://github.com/bigeasy/cadence package, it wraps callbacks in a function that has zero parameters, so the test on line 250 would fail. now it doesn't!

WebReflection commented 9 years ago

hi there, you changed the library logic and everything is failing.

if (1 < callback.length) {

above snippet means that if the callback has more than 1 argument expected, it should pass.

Your change here means:

if (1 !== callback.length) {

if the callback has a length different from one, even less than 1, it should pass.

This breaks the contract that callbacks with a length more than one should pass, and all others should fallback.

So, what are you trying to do?

mrose17 commented 9 years ago

wow! that's fast.

sorry, i didn't realize there was a test suite; otherwise, i would have fixed it.

here's the issue: i use a package called cadence (https://github.com/bigeasy/cadence) when i make a call using cadence, something like:

    var dblite = require('dblite')
        , db = dblite(path)

    async(function () {
        db.query(query, [ params ], async())
    }, function (rows) {
      ...
    }

the callback that is given to db.query appears to have zero parameters, but it really doesn't. so the logic in the current package fails when using cadence.

i'm not sure how to resolve this frankly. i'll talk with the cadence author and see what ideas he has.

mrose17 commented 9 years ago

cf., https://github.com/bigeasy/cadence/issues/322

WebReflection commented 9 years ago

the callback that is given to db.query appears to have zero parameters, but it really doesn't

well, in JavaScript, the function length is defined statically and reflected.

function zero() {} // length 0
function one(one) {} // length 1
function two(one, two) {} // length 2

no matter how much magic you do with arguments

The problem you encountered is because of a migration pattern between old style and new one.

Last, but not least, of course there is a test suite, what kind of library wouldn't have one ;-)

Regards

mrose17 commented 9 years ago

i don't have the competence to muck around with cadence, although i work with the package author and when he gets some time, he'll probably write a one-line explanation showing me how to make the two packages work together (hence the issue cf.'d above).

all i can tell you for a fact is that with the existing code, using async() as the callback parameter to db.fetch results in the else path on https://github.com/WebReflection/dblite/blob/master/build/dblite.node.js#L354 is invoked, despite the fact that the value passed as the callback is an error-first function...

WebReflection commented 9 years ago

it's not, or better, someone is magically making it as such through arguments and the length, but the function signature has no more than 1 parameters defined ;-)

Looking forward to read your mate explanation, and possible fix.

Best Regards

mrose17 commented 9 years ago

many thanks for the speedy reply and analysis...

cheers!