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

Remove bluebird.using #3552

Merged
merged 6 commits into from Nov 28, 2019
Merged

Remove bluebird.using #3552

merged 6 commits into from Nov 28, 2019

Conversation

@maximelkin
Copy link
Contributor

maximelkin commented Nov 27, 2019

No description provided.

@maximelkin

This comment has been minimized.

Copy link
Contributor Author

maximelkin commented Nov 27, 2019

I dont understand acquireConnection implementation:

  1. in default implementation it declared with arguments (client, config, txid)
  2. also it called in same file with same arguments
  3. this method also overriden in mssql, oracle, oracledb, but they have only one argument - config
  4. also there are calls to Client.prototype.acquireConnection, but this not so interesting
@maximelkin maximelkin force-pushed the maximelkin:bluebird_remove_using branch from eccfd34 to e5eaa91 Nov 27, 2019
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 27, 2019

@maximelkin Probably it works almost by accident.
See in mssql, for an example:

const configConnection = config && config.connection; // this will fail to resolve because client is passed instead of a config
<...>
      try {
        resolve(
          (t.outerTx ? t.outerTx.conn : null) ||
            configConnection ||
            t.client.acquireConnection()
        );
      }
// we will resolve this with a client mistakenly marked as configConnection 
<...>
conn.tx_ = conn.transaction(); // luckily for us, `client.js` has `transaction()` method defined, so probably that works and the whole monster works

Frankly, I think this is wrong and should be refactored ruthlessly.

@@ -79,9 +79,7 @@ Object.assign(Runner.prototype, {
const stream = new PassThrough({ objectMode: true });

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 27, 2019

Collaborator

One using was missed at line 57.

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 27, 2019

Collaborator

Oh, wait, sorry, you only changed runner.js, transaction.js remains as-is.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 27, 2019

@maximelkin Opened #3554 to make sure that code is at least not lying - it could actually be that client.connection is what was intended to be consumed in the first place, so I didn't touch actual implementation yet, since it seems to be working. If CI passes, I'll merge it so that it's less confusing.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 27, 2019

@maximelkin #3554 was merged, you can rebase to de-confuse code :D

@maximelkin maximelkin force-pushed the maximelkin:bluebird_remove_using branch from e5eaa91 to 72cfa4c Nov 27, 2019
@@ -208,32 +205,32 @@ class Transaction extends EventEmitter {
// Acquire a connection and create a disposer - either using the one passed
// via config or getting one off the client. The disposer will be called once
// the original promise is marked completed.
acquireConnection(client, config, txid) {
acquireConnection(config, cb) {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 27, 2019

Collaborator

why get rid of txid? it was always passed from this.txid anyway?

This comment has been minimized.

Copy link
@maximelkin

maximelkin Nov 27, 2019

Author Contributor

Yes

This comment has been minimized.

@@ -22,7 +22,7 @@ Object.assign(Runner.prototype, {
run() {
const runner = this;
return (
Bluebird.using(this.ensureConnection(), function(connection) {
this.ensureConnection(function(connection) {

This comment has been minimized.

Copy link
@maximelkin

maximelkin Nov 28, 2019

Author Contributor

return native promise, conversion to bluebird promise done in lib/interface.js

conn.__knexTxId = t.txid;
if (!t.outerTx) {
t.conn = conn;
.then((conn) => {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 28, 2019

Collaborator

would be awesome to convert those then chains into async/await, but separate PR would be better for that.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 28, 2019

Looks fantastic, thank you!

@kibertoad kibertoad merged commit 84fce33 into knex:master Nov 28, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@maximelkin maximelkin deleted the maximelkin:bluebird_remove_using branch Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.