broken with knex@0.20.11 since it no longer includes Bluebird

[kriscarle]

Error message is:

UnhandledPromiseRejectionWarning: TypeError: self.knex.select(...).from(...).where(...).andWhereRaw(...).then(...).asCallback is not a function
    at /node_modules/connect-session-knex/index.js:264:5

See https://github.com/knex/knex/issues/3704

[machineghost]

For anyone looking for a quick fix (not permanent) to this issue:

- "connect-session-knex": "^1.5.0",
+ "connect-session-knex": "git@github.com:machineghost/connect-session-knex.git",

Basically it just copy/pastes the missing Bluebird function in, to avoid adding a dependency on the whole library.

It's a dead end repo, so I'm only mentioning it as a temporary fix (please don't change your package.json permanently!) Hopefully the library maintainer will either accept it, or another fix soon.

[machineghost]

I realized my attempt to avoid Bluebird was flawed, as a proper recreation of asCallback would require copying a lot more code.

Rather than do that I redid things to instead depend on Bluebird, which let me convert every foo.asCallback into a resolve(foo).asCallback (resolve is Bluebird-speak for "convert into a Bluebird promise").

P.S. Unfortunately I don't have MySQL setup so I can't run the tests.

[jehy]

What's the reason for removing bluebird? It's still faster then native promises, and has rich syntax. Things like https://github.com/llambda/connect-session-knex/pull/62 just look strange - you avoid the library for no reason, break things and then copy-paste parts of library back...

[kibertoad]

@kriscarle Why not just use https://nodejs.org/api/util.html#util_util_callbackify_original?

[kibertoad]

@jehy See https://blog.kuzzle.io/bluebird-vs-native-vs-async/await-state-of-promises-performances-in-2019. In modern Node (12+) there are basically no performance benefits to use Bluebird vs native async/await, and using non-standard solutions is always a pain, especially since your users are likely to get coupled to the non-standard part of it (as evidenced by amount of issues users are currently having when Bluebird is gone). I understand the pains of migration, and I am sorry for them, but this is not a new or unexpected development - we stopped officially supporting Bluebird since 0.18.0 (https://github.com/knex/knex/blob/master/UPGRADING.md)

[jehy]

@kibertoad yup, I saw that benchmark. Also I saw opinion that

In long term I hope every promise library would be obsolete.

But IMHO there are two wrong assumptions: 1) Bluebird is used bacause it's faster. 2) Bluebird is used as a pollyfill.

I think that's not the case for really complex applications, which go beyond async-await one liners. Bluebird has many features that are absolutely necessary for complex async flow - like timeouts, custom error handling, mapping with concurrency, cancellation, reducing and so on. All those features can be implemented in native promises, but that's a lot of useless boilerplate.

Suggested "solution" with wrapping all calls with bluebird Promise.resolve sounds really bad, especially if you are dealing with a big service where you'll have to rewrite tons of code with this ugly wrapper.

Let's take a look at ioredis. There was also a talk about supporting native promises - and guys made a really good solution - they added an option to support any custom promise library and they use native promise by default. Why not? Setting a custom promise library is fast and doesn't require patching all application code.

[machineghost]

Suggested "solution" with wrapping all calls with bluebird Promise.resolve sounds really bad, especially if you are dealing with a big service where you'll have to rewrite tons of code with this ugly wrapper.

I don't disagree in the slightest. My only counter would be that a slow (but working) library is superior to a non-working one (ie. the state for the past five days) ... but of course a fast working library would be best.

I have no familiarity with ioredis though, so someone else will have to PR an implementation based on it.

[jehy]

@machineghost Sorry, when I was talking about wrapping every call in Promise.resolve - I meant suggested workaround from knex itself, not your fix. Your fix is fine :)

[gx0r]

Thank you for the bug report and fix! I published a new version!

[kibertoad]

@llambda Are there any reasons why native callbackify (https://nodejs.org/api/util.html#util_util_callbackify_original) couldn't be used instead? It is natively supported in Node 8 and it's way less hacky than Bluebird wrapping.

You can see an example for how to convert Promise using that approach here: https://stackoverflow.com/questions/40640623/how-to-convert-promise-into-callback-in-node-js

[gx0r]

@llambda Are there any reasons why native callbackify (https://nodejs.org/api/util.html#util_util_callbackify_original) couldn't be used instead? It is natively supported in Node 8 and it's way less hacky than Bluebird wrapping.

You can see an example for how to convert Promise using that approach here: https://stackoverflow.com/questions/40640623/how-to-convert-promise-into-callback-in-node-js

I think I'd be open to that change, if someone were to put together a merge request, otherwise I may do it in my own time.

[machineghost]

FYI the reason I went with a Bluebird approach is that I wasn't sure if there was any "magic" in Bluebird that wasn't in Node's version, and my goal was just to get the library working again as quickly as possible without breaking anything.

I 100% agree that callbackify is a better approach ... it just requires that someone actually test and confirm that no "Bluebird magic" gets lost in the process (and since I don't have MySQL setup I also couldn't do that testing).