refactor: replace usage of Bluebird Promise.tap with native Promise.then #3287
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@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 :) |
kibertoad
reviewed
Jun 17, 2019
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); | |||
There was a problem hiding this comment.
Isn't this part going to mess up with return functionality? Probably return call should be replace with await here.
|
@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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.