sk2 - npm Package Compare versions

Comparing version 1.0.2 to 1.0.3

index.js

@@ -83,3 +83,3 @@ const { map } = require('lodash');
     const instance = Knex({ client: Client });
-    instance.queryBuilder = instance.context.queryBuilder = function() {
+    instance.context.queryBuilder = function() {
       return this.client.queryBuilder().options({ transaction: tx });
@@ -89,8 +89,10 @@ };
     instance.rollback = () => tx.rollback();
-    if (container) {
+
+    const result = container(instance);
+    if (result && typeof result.then === 'function') {
       try {
-        const resp = await container(instance);
+        const resp = await result;
         await tx.commit();
         return resp;
-      } catch(err) {
+      } catch (err) {
         await tx.rollback();
@@ -100,2 +102,3 @@ throw err;
     }
+
     return instance;
@@ -123,4 +126,2 @@ };
 
-  knex.transaction = transaction;
-
   return knex;
@@ -127,0 +128,0 @@ };

lib/dialects/mssql/transaction.js

@@ -58,46 +58,36 @@ const Transaction = require('../../transaction');
   // the original promise is marked completed.
-  acquireConnection(config, cb) {
+  async acquireConnection(config, cb) {
     const configConnection = config && config.connection;
-    return new Promise((resolve, reject) => {
-      try {
-        resolve(
-          (this.outerTx ? this.outerTx.conn : null) ||
-            configConnection ||
-            this.client.acquireConnection()
-        );
-      } catch (e) {
-        reject(e);
+    const conn =
+      (this.outerTx && this.outerTx.conn) ||
+      configConnection ||
+      (await this.client.acquireConnection());
+
+    try {
+      conn.__knexTxId = this.txid;
+      if (!this.outerTx) {
+        this.conn = conn;
+        conn.tx_ = conn.transaction();
       }
-    })
-      .then((conn) => {
-        conn.__knexTxId = this.txid;
-        if (!this.outerTx) {
-          this.conn = conn;
-          conn.tx_ = conn.transaction();
-        }
-        return conn;
-      })
-      .then(async (conn) => {
-        try {
-          return await cb(conn);
-        } finally {
-          if (!this.outerTx) {
-            if (conn.tx_) {
-              if (!this._completed) {
-                debug('%s: unreleased transaction', this.txid);
-                conn.tx_.rollback();
-              }
-              conn.tx_ = null;
-            }
-            this.conn = null;
-            if (!configConnection) {
-              debug('%s: releasing connection', this.txid);
-              this.client.releaseConnection(conn);
-            } else {
-              debug('%s: not releasing external connection', this.txid);
-            }
+
+      return await cb(conn);
+    } finally {
+      if (!this.outerTx) {
+        if (conn.tx_) {
+          if (!this._completed) {
+            debug('%s: unreleased transaction', this.txid);
+            conn.tx_.rollback();
           }
+          conn.tx_ = null;
         }
-      });
+        this.conn = null;
+        if (!configConnection) {
+          debug('%s: releasing connection', this.txid);
+          this.client.releaseConnection(conn);
+        } else {
+          debug('%s: not releasing external connection', this.txid);
+        }
+      }
+    }
   }
 };

lib/dialects/oracledb/transaction.js

@@ -53,38 +53,27 @@ const { isUndefined } = require('lodash');
 
-  acquireConnection(config, cb) {
+  async acquireConnection(config, cb) {
     const configConnection = config && config.connection;
-    const t = this;
-    return new Promise((resolve, reject) => {
+
+    const connection =
+      configConnection || (await this.client.acquireConnection());
+    try {
+      connection.__knexTxId = this.txid;
+      connection.isTransaction = true;
+      return await cb(connection);
+    } finally {
+      debugTx('%s: releasing connection', this.txid);
+      connection.isTransaction = false;
       try {
-        this.client
-          .acquireConnection()
-          .then((cnx) => {
-            cnx.__knexTxId = this.txid;
-            cnx.isTransaction = true;
-            resolve(cnx);
-          })
-          .catch(reject);
-      } catch (e) {
-        reject(e);
-      }
-    }).then(async (connection) => {
-      try {
-        return await cb(connection);
+        await connection.commitAsync();
+      } catch (err) {
+        this._rejecter(err);
       } finally {
-        debugTx('%s: releasing connection', this.txid);
-        connection.isTransaction = false;
-        try {
-          await connection.commitAsync();
-        } catch (err) {
-          t._rejecter(err);
-        } finally {
-          if (!configConnection) {
-            await t.client.releaseConnection(connection);
-          } else {
-            debugTx('%s: not releasing external connection', t.txid);
-          }
+        if (!configConnection) {
+          await this.client.releaseConnection(connection);
+        } else {
+          debugTx('%s: not releasing external connection', this.txid);
         }
       }
-    });
+    }
   }
 };

lib/transaction.js

@@ -40,2 +40,3 @@ // Transaction
     this.trxClient = undefined;
+    this._completed = false;
     this._debug = client.config && client.config.debug;
@@ -49,2 +50,8 @@
 
+    // `this` can potentially serve as an `outerTx` for another
+    // Transaction.  So, go ahead and establish `_lastChild` now.
+    this._lastChild = Promise.resolve();
+
+    const _previousSibling = outerTx ? outerTx._lastChild : Promise.resolve();
+
     // FYI: As you will see in a moment, this Promise will be used to construct
@@ -54,3 +61,5 @@ //      2 separate Promise Chains.  This ensures that each Promise Chain
     //      with the other Promise Chain.
-    const basePromise = this._evaluateContainer(config, container);
+    const basePromise = _previousSibling.then(() =>
+      this._evaluateContainer(config, container)
+    );
 
@@ -61,12 +70,3 @@ // FYI: This is the Promise Chain for EXTERNAL use.  It ensures that the
 
-    this._completed = false;
-
-    // If there's a wrapping transaction, we need to wait for any older sibling
-    // transactions to settle (commit or rollback) before we can start, and we
-    // need to register ourselves with the parent transaction so any younger
-    // siblings can wait for us to complete before they can start.
-    this._previousSibling = Promise.resolve(true);
     if (outerTx) {
-      if (outerTx._lastChild) this._previousSibling = outerTx._lastChild;
-
       // FYI: This is the Promise Chain for INTERNAL use.  It serves as a signal
@@ -162,10 +162,2 @@ //      for when the next sibling should begin its execution.  Therefore,
   async _evaluateContainer(config, container) {
-    // FYI: This is temporarily stalling things so that the constructor
-    //      can finish initializing the Transaction.  Otherwise,
-    //      `this.previousSibling` will still be `undefined`.
-    await Promise.resolve();
-
-    // Wait for the earlier Transactions to complete before proceeding.
-    await this._previousSibling;
-
     return this.acquireConnection(config, (connection) => {
@@ -223,23 +215,18 @@ const trxClient = (this.trxClient = makeTxClient(
   // the original promise is marked completed.
-  acquireConnection(config, cb) {
+  async acquireConnection(config, cb) {
     const configConnection = config && config.connection;
-    return new Promise((resolve, reject) => {
-      try {
-        resolve(configConnection || this.client.acquireConnection());
-      } catch (e) {
-        reject(e);
+    const connection =
+      configConnection || (await this.client.acquireConnection());
+
+    try {
+      connection.__knexTxId = this.txid;
+      return await cb(connection);
+    } finally {
+      if (!configConnection) {
+        debug('%s: releasing connection', this.txid);
+        this.client.releaseConnection(connection);
+      } else {
+        debug('%s: not releasing external connection', this.txid);
       }
-    }).then(async (connection) => {
-      try {
-        connection.__knexTxId = this.txid;
-        return await cb(connection);
-      } finally {
-        if (!configConnection) {
-          debug('%s: releasing connection', this.txid);
-          this.client.releaseConnection(connection);
-        } else {
-          debug('%s: not releasing external connection', this.txid);
-        }
-      }
-    });
+    }
   }
@@ -269,3 +256,3 @@
 
-  transactor.withUserParams = () => {
+  transactor.context.withUserParams = () => {
     throw new Error(
@@ -279,3 +266,3 @@ 'Cannot set user params on a transaction - it can only inherit params from main knex instance'
 
-  transactor.transaction = function(container, options) {
+  transactor.context.transaction = function(container, options) {
     if (!options) {
@@ -287,16 +274,5 @@ options = { doNotRejectOnRollback: true };
 
-    if (container) {
-      return trxClient.transaction(container, options, trx);
-    } else {
-      return new Promise((resolve, _reject) => {
-        trxClient.transaction(
-          (nestedTrx) => {
-            resolve(nestedTrx);
-          },
-          options,
-          trx
-        );
-      });
-    }
+    return this._transaction(container, options, trx);
   };
+
   transactor.savepoint = function(container, options) {
@@ -303,0 +279,0 @@ return transactor.transaction(container, options);

lib/util/make-knex.js

@@ -10,2 +10,88 @@ const { EventEmitter } = require('events');
 
+// Javascript does not officially support "callable objects".  Instead,
+// you must create a regular Function and inject properties/methods
+// into it.  In other words: you can't leverage Prototype Inheritance
+// to share the property/method definitions.
+//
+// To work around this, we're creating an Object Property Definition.
+// This allow us to quickly inject everything into the `knex` function
+// via the `Object.defineProperties(..)` function.  More importantly,
+// it allows the same definitions to be shared across `knex` instances.
+const KNEX_PROPERTY_DEFINITIONS = {
+  client: {
+    get() {
+      return this.context.client;
+    },
+    set(client) {
+      this.context.client = client;
+    },
+    configurable: true,
+  },
+
+  userParams: {
+    get() {
+      return this.context.userParams;
+    },
+    set(userParams) {
+      this.context.userParams = userParams;
+    },
+    configurable: true,
+  },
+
+  schema: {
+    get() {
+      return this.client.schemaBuilder();
+    },
+    configurable: true,
+  },
+
+  migrate: {
+    get() {
+      return new Migrator(this);
+    },
+    configurable: true,
+  },
+
+  seed: {
+    get() {
+      return new Seeder(this);
+    },
+    configurable: true,
+  },
+
+  fn: {
+    get() {
+      return new FunctionHelper(this.client);
+    },
+    configurable: true,
+  },
+};
+
+// `knex` instances serve as proxies around `context` objects.  So, calling
+// any of these methods on the `knex` instance will forward the call to
+// the `knex.context` object. This ensures that `this` will correctly refer
+// to `context` within each of these methods.
+const CONTEXT_METHODS = [
+  'raw',
+  'batchInsert',
+  'transaction',
+  'transactionProvider',
+  'initialize',
+  'destroy',
+  'ref',
+  'withUserParams',
+  'queryBuilder',
+  'disableProcessing',
+  'enableProcessing',
+];
+
+for (const m of CONTEXT_METHODS) {
+  KNEX_PROPERTY_DEFINITIONS[m] = {
+    value: function(...args) {
+      return this.context[m](...args);
+    },
+    configurable: true,
+  };
+}
+
 function makeKnex(client) {
@@ -42,4 +128,4 @@ // The object we're potentially using to kick off an initial chain.
       const config = Object.assign({}, _config);
-      config.userParams = this.userParams || {}
-      if(isUndefined(config.doNotRejectOnRollback)) {
+      config.userParams = this.userParams || {};
+      if (isUndefined(config.doNotRejectOnRollback)) {
         // Backwards-compatibility: default value changes depending upon
@@ -50,9 +136,14 @@ // whether or not a `container` was provided.
 
+      return this._transaction(container, config);
+    },
 
-      if(container) {
-        const trx = this.client.transaction(container, config);
+    // Internal method that actually establishes the Transaction.  It makes no assumptions
+    // about the `config` or `outerTx`, and expects the caller to handle these details.
+    _transaction(container, config, outerTx = null) {
+      if (container) {
+        const trx = this.client.transaction(container, config, outerTx);
         return trx;
       } else {
         return new Promise((resolve, reject) => {
-          const trx = this.client.transaction(resolve, config);
+          const trx = this.client.transaction(resolve, config, outerTx);
           trx.catch(reject);
@@ -144,5 +235,29 @@ });
   // any other information is specified.
+  //
+  // TODO: `QueryBuilder.extend(..)` allows new QueryBuilder
+  //       methods to be introduced via external components.
+  //       As a side-effect, it also pushes the new method names
+  //       into the `QueryInterface` array.
+  //
+  //       The Problem: due to the way the code is currently
+  //       structured, these new methods cannot be retroactively
+  //       injected into existing `knex` instances!  As a result,
+  //       some `knex` instances will support the methods, and
+  //       others will not.
+  //
+  //       We should revisit this once we figure out the desired
+  //       behavior / usage.  For instance: do we really want to
+  //       allow external components to directly manipulate `knex`
+  //       data structures?  Or, should we come up w/ a different
+  //       approach that avoids side-effects / mutation?
+  //
+  //      (FYI: I noticed this issue because I attempted to integrate
+  //       this logic directly into the `KNEX_PROPERTY_DEFINITIONS`
+  //       construction.  However, `KNEX_PROPERTY_DEFINITIONS` is
+  //       constructed before any `knex` instances are created.
+  //       As a result, the method extensions were missing from all
+  //       `knex` instances.)
   QueryInterface.forEach(function(method) {
     knex[method] = function() {
-      const builder = knex.queryBuilder();
+      const builder = this.queryBuilder();
       return builder[method].apply(builder, arguments);
@@ -152,78 +267,11 @@ };
 
-  Object.defineProperties(knex, {
-    context: {
-      get() {
-        return knex._context;
-      },
-      set(context) {
-        knex._context = context;
+  Object.defineProperties(knex, KNEX_PROPERTY_DEFINITIONS);
 
-        // Redefine public API for knex instance that would be proxying methods from correct context
-        knex.raw = context.raw;
-        knex.batchInsert = context.batchInsert;
-        knex.transaction = context.transaction;
-        knex.transactionProvider = context.transactionProvider;
-        knex.initialize = context.initialize;
-        knex.destroy = context.destroy;
-        knex.ref = context.ref;
-        knex.withUserParams = context.withUserParams;
-        knex.queryBuilder = context.queryBuilder;
-        knex.disableProcessing = context.disableProcessing;
-        knex.enableProcessing = context.enableProcessing;
-      },
-      configurable: true,
-    },
-
-    client: {
-      get() {
-        return knex.context.client;
-      },
-      set(client) {
-        knex.context.client = client;
-      },
-      configurable: true,
-    },
-
-    userParams: {
-      get() {
-        return knex.context.userParams;
-      },
-      set(userParams) {
-        knex.context.userParams = userParams;
-      },
-      configurable: true,
-    },
-
-    schema: {
-      get() {
-        return knex.client.schemaBuilder();
-      },
-      configurable: true,
-    },
-
-    migrate: {
-      get() {
-        return new Migrator(knex);
-      },
-      configurable: true,
-    },
-
-    seed: {
-      get() {
-        return new Seeder(knex);
-      },
-      configurable: true,
-    },
-
-    fn: {
-      get() {
-        return new FunctionHelper(knex.client);
-      },
-      configurable: true,
-    },
-  });
-
   initContext(knex);
   knex.client = client;
+
+  // TODO: It looks like this field is never actually used.
+  //       It should probably be removed in a future PR.
   knex.client.makeKnex = makeKnex;
+
   knex.userParams = {};
@@ -291,3 +339,3 @@
   Object.assign(clonedFunction, originalFunction);
-  clonedFunction._context = knexContext;
+  clonedFunction.context = knexContext;
   return clonedFunction;
@@ -294,0 +342,0 @@ }

package.json

 {
   "name": "sk2",
-  "version": "1.0.2",
+  "version": "1.0.3",
   "description": "knex with sequelize query",
@@ -37,11 +37,11 @@ "main": "index.js",
   "devDependencies": {
-    "@babel/parser": "^7.8.7",
+    "@babel/parser": "^7.8.8",
     "@babel/traverse": "^7.8.6",
     "chai": "^4.2.0",
-    "coveralls": "^3.0.9",
-    "fs-extra": "^8.1.0",
+    "coveralls": "^3.0.11",
+    "fs-extra": "^9.0.0",
     "glob": "^7.1.6",
-    "knex": "^0.20.11",
+    "knex": "^0.20.12",
     "mariadb": "^2.2.0",
-    "mocha": "^7.1.0",
+    "mocha": "^7.1.1",
     "mysql2": "^2.1.0",
@@ -64,6 +64,6 @@ "nyc": "^15.0.0",
     "mkdirp": "^1.0.3",
-    "pg-connection-string": "2.1.0",
+    "pg-connection-string": "^2.2.0",
     "tarn": "^2.0.0",
-    "uuid": "^7.0.1"
+    "uuid": "^7.0.2"
   }
 }

No alert changes

Improved metrics

Total package byte prevSize

466451

increased by0.13%

Worsened metrics

Lines of code

14092

decreased by-0.01%

Dependency changes

• + Addedpg-connection-string@2.7.0(transitive)

• - Removedpg-connection-string@2.1.0(transitive)

• Updatedpg-connection-string@^2.2.0

• Updateduuid@^7.0.2