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
merged 3 commits into from Jun 17, 2019

Conversation

Projects
None yet
2 participants
@benrki
Copy link
Contributor

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.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

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

@@ -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);

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jun 17, 2019

Collaborator

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

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 tgriesser:master Jun 17, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 88.185%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.