broken with knex@0.20.11 since it no longer includes Bluebird #61
Comments
Since the problem was a reliance on Bluebird's asCallback, I just copied that library's implementation, along with a slight tweak to use "assert" instead of "./assert", and to not modify the prototype.
|
For anyone looking for a quick fix (not permanent) to this issue: 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. |
This reverts commit 8d9cdfb.
|
I realized my attempt to avoid Bluebird was flawed, as a proper recreation of Rather than do that I redid things to instead depend on Bluebird, which let me convert every P.S. Unfortunately I don't have MySQL setup so I can't run the tests. |
|
What's the reason for removing bluebird? It's still faster then native promises, and has rich syntax. |
|
@kriscarle Why not just use https://nodejs.org/api/util.html#util_util_callbackify_original? |
|
@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). |
|
@kibertoad yup, I saw that benchmark. Also I saw opinion that
But IMHO there are two wrong assumptions:
I think that's not the case for really complex applications, which go beyond async-await one liners. 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. |
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. |
#62) * Slightly kludgy fix for #61 Since the problem was a reliance on Bluebird's asCallback, I just copied that library's implementation, along with a slight tweak to use "assert" instead of "./assert", and to not modify the prototype. * Fixed reference to "self" to instead use "this" (in the only place in the code that didn't have a "self" variable) * Revert "Fixed reference to "self" to instead use "this" (in the only place in the code that didn't have a "self" variable)" This reverts commit 88d1a96. * Revert "Slightly kludgy fix for #61" This reverts commit 8d9cdfb. * Undid previous approach and went with a Bluebird-dependent approach
|
@machineghost Sorry, when I was talking about wrapping every call in |
|
Thank you for the bug report and fix! I published a new version! |
|
@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. |
|
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). |
#62) * Slightly kludgy fix for #61 Since the problem was a reliance on Bluebird's asCallback, I just copied that library's implementation, along with a slight tweak to use "assert" instead of "./assert", and to not modify the prototype. * Fixed reference to "self" to instead use "this" (in the only place in the code that didn't have a "self" variable) * Revert "Fixed reference to "self" to instead use "this" (in the only place in the code that didn't have a "self" variable)" This reverts commit 88d1a96. * Revert "Slightly kludgy fix for #61" This reverts commit 8d9cdfb. * Undid previous approach and went with a Bluebird-dependent approach
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:5See knex/knex#3704
The text was updated successfully, but these errors were encountered: