Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: replace usage of Bluebird Promise.tap with native Promise.then #3287

merged 3 commits into from Jun 17, 2019


Copy link

@benrki benrki commented Jun 17, 2019

fixes #3255

Alternative solution: implement a version of .tap in a Promise wrapper around the native Promise (or a providable A+ compliant library) instead of Bluebird.js was done with src/promise.js previously.

I opted to always return the resolved promise value even in cases where it was not used in case it might be extended later in the promise chain.

Copy link

@kibertoad kibertoad commented Jun 17, 2019

@benrki Personally I would prefer to avoid extending standard Promise functionality as much as possible, since otherwise we would be moving from one custom Promise solution to another, except less tested and polished one, so I think that this is indeed a good approach :)

src/client.js Outdated
@@ -251,12 +251,13 @@ assign(Client.prototype, {

return Object.assign(poolConfig, {
create: () => {
return this.acquireRawConnection().tap((connection) => {
return this.acquireRawConnection().then((connection) => {
connection.__knexUid = uniqueId('__knexUid');

if (poolConfig.afterCreate) {
return Bluebird.promisify(poolConfig.afterCreate)(connection);
Copy link

@kibertoad kibertoad Jun 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this part going to mess up with return functionality? Probably return call should be replace with await here.

Copy link

@kibertoad kibertoad commented Jun 17, 2019

@benrki Thank you for your contribution! In two cases there were missing awaits for interim promises, but I believe I've got them all now. Let's see if CI passes :D

@kibertoad kibertoad merged commit 9d8e900 into knex:master Jun 17, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

2 participants