CLI cleanup. Do not require client for creating seeds or migrations (#2905)

* Specify jakefile explicitly to ensure it being run. Do not require client when creating seeds or migrations. cli.js cleanup. Formatting

* Fix message

* Fix typo

* Ignore console rule in CLI tests

* Fix rimraf import

* Improvements after code review

* One more arrow function

bin/cli.js

@@ -1,16 +1,16 @@
 #!/usr/bin/env node
 /* eslint no-console:0, no-var:0 */
-var Liftoff = require('liftoff');
-var Promise = require('bluebird');
-var interpret = require('interpret');
-var path = require('path');
-var chalk = require('chalk');
-var tildify = require('tildify');
-var commander = require('commander');
-var argv = require('minimist')(process.argv.slice(2));
-var fs = Promise.promisifyAll(require('fs'));
-var cliPkg = require('../package');
-var { isObject } = require('lodash');
+const Liftoff = require('liftoff');
+const Promise = require('bluebird');
+const interpret = require('interpret');
+const path = require('path');
+const chalk = require('chalk');
+const tildify = require('tildify');
+const commander = require('commander');
+const argv = require('minimist')(process.argv.slice(2));
+const fs = Promise.promisifyAll(require('fs'));
+const cliPkg = require('../package');
+const { resolveClientNameWithAliases } = require('../lib/helpers');
 
 function exit(text) {
   if (text instanceof Error) {
@@ -37,8 +37,9 @@ function checkLocalModule(env) {
 }
 
 function mkConfigObj(opts) {
-  let envName = opts.env || process.env.NODE_ENV || 'development';
-  let useNullAsDefault = opts.client === 'sqlite3';
+  const envName = opts.env || process.env.NODE_ENV || 'development';
+  const resolvedClientName = resolveClientNameWithAliases(opts.client);
+  const useNullAsDefault = resolvedClientName === 'sqlite3';
   return {
     ext: 'js',
     [envName]: {
@@ -55,12 +56,18 @@ function mkConfigObj(opts) {
 function initKnex(env, opts) {
   checkLocalModule(env);
 
-  if (!env.configuration) {
-    if (opts.client) env.configuration = mkConfigObj(opts);
-    else
+  if (!env.pathToKnexFile) {
+    if (opts.client) {
+      env.configuration = mkConfigObj(opts);
+    } else {
       exit(
-        'No knexfile found in this directory. Specify a path with --knexfile'
+        'No knexfile found in this directory. Specify a path with --knexfile or pass --client and --connection params in commandline'
       );
+    }
+  }
+  // If knexfile is specified
+  else {
+    env.configuration = require(env.pathToKnexFile);
   }
 
   if (process.cwd() !== env.cwd) {
@@ -71,11 +78,10 @@ function initKnex(env, opts) {
     );
   }
 
-  var environment = opts.env || process.env.NODE_ENV;
-  var defaultEnv = 'development';
-  var config = isObject(env.configuration)
-    ? env.configuration
-    : require(env.configuration);
+  let environment = opts.env || process.env.NODE_ENV;
+  const defaultEnv = 'development';
+
+  let config = env.configuration;
 
   if (!environment && typeof config[defaultEnv] === 'object') {
     environment = defaultEnv;
@@ -92,14 +98,14 @@ function initKnex(env, opts) {
   }
 
   if (argv.debug !== undefined) config.debug = argv.debug;
-  var knex = require(env.modulePath);
+  const knex = require(env.modulePath);
   return knex(config);
 }
 
 function invoke(env) {
   env.modulePath = env.modulePath || process.env.KNEX_PATH;
-  var filetypes = ['js', 'coffee', 'ts', 'eg', 'ls'];
-  var pending = null;
+  const filetypes = ['js', 'coffee', 'ts', 'eg', 'ls'];
+  let pending = null;
 
   commander
     .version(
@@ -132,8 +138,8 @@ function invoke(env) {
       `-x [${filetypes.join('|')}]`,
       'Specify the knexfile extension (default js)'
     )
-    .action(function() {
-      var type = (argv.x || 'js').toLowerCase();
+    .action(() => {
+      const type = (argv.x || 'js').toLowerCase();
       if (filetypes.indexOf(type) === -1) {
         exit(`Invalid filetype specified: ${type}`);
       }
@@ -141,7 +147,7 @@ function invoke(env) {
         exit(`Error: ${env.configuration} already exists`);
       }
       checkLocalModule(env);
-      var stubPath = `./knexfile.${type}`;
+      const stubPath = `./knexfile.${type}`;
       pending = fs
         .readFileAsync(
           path.dirname(env.modulePath) +
@@ -149,10 +155,10 @@ function invoke(env) {
             type +
             '.stub'
         )
-        .then(function(code) {
+        .then((code) => {
           return fs.writeFileAsync(stubPath, code);
         })
-        .then(function() {
+        .then(() => {
           success(chalk.green(`Created ${stubPath}`));
         })
         .catch(exit);
@@ -165,16 +171,14 @@ function invoke(env) {
       `-x [${filetypes.join('|')}]`,
       'Specify the stub extension (default js)'
     )
-    .action(function(name) {
-      var instance = initKnex(env, commander.opts());
-      var ext = (
-        argv.x ||
-        env.configuration.ext ||
-        env.configuration.split('.').pop()
-      ).toLowerCase();
+    .action((name) => {
+      const opts = commander.opts();
+      opts.client = opts.client || 'sqlite3'; // We don't really care about client when creating migrations
+      const instance = initKnex(env, opts);
+      const ext = (argv.x || env.configuration.ext).toLowerCase();
       pending = instance.migrate
         .make(name, { extension: ext })
-        .then(function(name) {
+        .then((name) => {
           success(chalk.green(`Created Migration: ${name}`));
         })
         .catch(exit);
@@ -183,10 +187,10 @@ function invoke(env) {
   commander
     .command('migrate:latest')
     .description('        Run all migrations that have not yet been run.')
-    .action(function() {
+    .action(() => {
       pending = initKnex(env, commander.opts())
         .migrate.latest()
-        .spread(function(batchNo, log) {
+        .spread((batchNo, log) => {
           if (log.length === 0) {
             success(chalk.cyan('Already up to date'));
           }
@@ -201,10 +205,10 @@ function invoke(env) {
   commander
     .command('migrate:rollback')
     .description('        Rollback the last set of migrations performed.')
-    .action(function() {
+    .action(() => {
       pending = initKnex(env, commander.opts())
         .migrate.rollback()
-        .spread(function(batchNo, log) {
+        .spread((batchNo, log) => {
           if (log.length === 0) {
             success(chalk.cyan('Already at the base migration'));
           }
@@ -220,10 +224,10 @@ function invoke(env) {
   commander
     .command('migrate:currentVersion')
     .description('        View the current version for the migration.')
-    .action(function() {
+    .action(() => {
       pending = initKnex(env, commander.opts())
         .migrate.currentVersion()
-        .then(function(version) {
+        .then((version) => {
           success(chalk.green('Current Version: ') + chalk.blue(version));
         })
         .catch(exit);
@@ -236,12 +240,14 @@ function invoke(env) {
       `-x [${filetypes.join('|')}]`,
       'Specify the stub extension (default js)'
     )
-    .action(function(name) {
-      var instance = initKnex(env, commander.opts());
-      var ext = (argv.x || env.configuration.split('.').pop()).toLowerCase();
+    .action((name) => {
+      const opts = commander.opts();
+      opts.client = opts.client || 'sqlite3'; // We don't really care about client when creating seeds
+      const instance = initKnex(env, opts);
+      const ext = (argv.x || env.configuration.ext).toLowerCase();
       pending = instance.seed
         .make(name, { extension: ext })
-        .then(function(name) {
+        .then((name) => {
           success(chalk.green(`Created seed file: ${name}`));
         })
         .catch(exit);
@@ -250,10 +256,10 @@ function invoke(env) {
   commander
     .command('seed:run')
     .description('        Run seed files.')
-    .action(function() {
+    .action(() => {
       pending = initKnex(env, commander.opts())
         .seed.run()
-        .spread(function(log) {
+        .spread((log) => {
           if (log.length === 0) {
             success(chalk.cyan('No seed files exist'));
           }
@@ -268,13 +274,13 @@ function invoke(env) {
 
   commander.parse(process.argv);
 
-  Promise.resolve(pending).then(function() {
+  Promise.resolve(pending).then(() => {
     commander.outputHelp();
     exit('Unknown command-line options, exiting');
   });
 }
 
-var cli = new Liftoff({
+const cli = new Liftoff({
   name: 'knex',
   extensions: interpret.jsVariants,
   v8flags: require('v8flags'),
@@ -291,7 +297,7 @@ cli.on('requireFail', function(name) {
 cli.launch(
   {
     cwd: argv.cwd,
-    configuration: argv.knexfile,
+    pathToKnexFile: argv.knexfile,
     require: argv.require,
     completion: argv.completion,
   },

src/helpers.js

@@ -7,6 +7,7 @@ import {
   isArray,
   isTypedArray,
 } from 'lodash';
+import { CLIENT_ALIASES } from './constants';
 
 // Check if the first argument is an array, otherwise uses all arguments as an
 // array.
@@ -61,3 +62,7 @@ export function addQueryContext(Target) {
     return this;
   };
 }
+
+export function resolveClientNameWithAliases(clientName) {
+  return CLIENT_ALIASES[clientName] || clientName;
+}

src/knex.js

@@ -4,7 +4,8 @@ import Client from './client';
 import makeKnex from './util/make-knex';
 import parseConnection from './util/parse-connection';
 import fakeClient from './util/fake-client';
-import { SUPPORTED_CLIENTS, CLIENT_ALIASES } from './constants';
+import { SUPPORTED_CLIENTS } from './constants';
+import { resolveClientNameWithAliases } from './helpers';
 
 export default function Knex(config) {
   // If config is a string, try to parse it
@@ -36,8 +37,8 @@ export default function Knex(config) {
       );
     }
 
-    Dialect = require(`./dialects/${CLIENT_ALIASES[clientName] ||
-      clientName}/index.js`);
+    const resolvedClientName = resolveClientNameWithAliases(clientName);
+    Dialect = require(`./dialects/${resolvedClientName}/index.js`);
   }
 
   // If config connection parameter is passed as string, try to parse it

src/seed/index.js

@@ -119,14 +119,16 @@ Seeder.prototype._waterfallBatch = function(seeds) {
   const seedDirectory = this._absoluteConfigDir();
   let current = Promise.bind({ failed: false, failedOn: 0 });
   const log = [];
-  each(seeds, function(seed) {
+  each(seeds, (seed) => {
     const name = path.join(seedDirectory, seed);
     seed = require(name);
 
     // Run each seed file.
-    current = current.then(() => seed.seed(knex, Promise)).then(function() {
-      log.push(name);
-    });
+    current = current
+      .then(() => seed.seed(knex, Promise))
+      .then(() => {
+        log.push(name);
+      });
   });
 
   return current.thenReturn([log]);

src/util/batchInsert.js

@@ -1,84 +1,84 @@
-import { isNumber, isArray, chunk, flatten, assign } from 'lodash';
-import Promise from 'bluebird';
-
-export default function batchInsert(
-  client,
-  tableName,
-  batch,
-  chunkSize = 1000
-) {
-  let returning = void 0;
-  let autoTransaction = true;
-  let transaction = null;
-
-  const getTransaction = () =>
-    new Promise((resolve, reject) => {
-      if (transaction) {
-        autoTransaction = false;
-        return resolve(transaction);
-      }
-
-      autoTransaction = true;
-      client.transaction(resolve).catch(reject);
-    });
-
-  const wrapper = assign(
-    new Promise((resolve, reject) => {
-      const chunks = chunk(batch, chunkSize);
-
-      if (!isNumber(chunkSize) || chunkSize < 1) {
-        return reject(new TypeError(`Invalid chunkSize: ${chunkSize}`));
-      }
-
-      if (!isArray(batch)) {
-        return reject(
-          new TypeError(`Invalid batch: Expected array, got ${typeof batch}`)
-        );
-      }
-
-      //Next tick to ensure wrapper functions are called if needed
-      return Promise.delay(1)
-        .then(getTransaction)
-        .then((tr) => {
-          return Promise.mapSeries(chunks, (items) =>
-            tr(tableName).insert(items, returning)
-          )
-            .then((result) => {
-              result = flatten(result || []);
-
-              if (autoTransaction) {
-                //TODO: -- Oracle tr.commit() does not return a 'thenable' !? Ugly hack for now.
-                return (tr.commit(result) || Promise.resolve()).then(
-                  () => result
-                );
-              }
-
-              return result;
-            })
-            .catch((error) => {
-              if (autoTransaction) {
-                return tr.rollback(error).then(() => Promise.reject(error));
-              }
-
-              return Promise.reject(error);
-            });
-        })
-        .then(resolve)
-        .catch(reject);
-    }),
-    {
-      returning(columns) {
-        returning = columns;
-
-        return this;
-      },
-      transacting(tr) {
-        transaction = tr;
-
-        return this;
-      },
-    }
-  );
-
-  return wrapper;
-}
+import { isNumber, isArray, chunk, flatten, assign } from 'lodash';
+import Promise from 'bluebird';
+
+export default function batchInsert(
+  client,
+  tableName,
+  batch,
+  chunkSize = 1000
+) {
+  let returning = void 0;
+  let autoTransaction = true;
+  let transaction = null;
+
+  const getTransaction = () =>
+    new Promise((resolve, reject) => {
+      if (transaction) {
+        autoTransaction = false;
+        return resolve(transaction);
+      }
+
+      autoTransaction = true;
+      client.transaction(resolve).catch(reject);
+    });
+
+  const wrapper = assign(
+    new Promise((resolve, reject) => {
+      const chunks = chunk(batch, chunkSize);
+
+      if (!isNumber(chunkSize) || chunkSize < 1) {
+        return reject(new TypeError(`Invalid chunkSize: ${chunkSize}`));
+      }
+
+      if (!isArray(batch)) {
+        return reject(
+          new TypeError(`Invalid batch: Expected array, got ${typeof batch}`)
+        );
+      }
+
+      //Next tick to ensure wrapper functions are called if needed
+      return Promise.delay(1)
+        .then(getTransaction)
+        .then((tr) => {
+          return Promise.mapSeries(chunks, (items) =>
+            tr(tableName).insert(items, returning)
+          )
+            .then((result) => {
+              result = flatten(result || []);
+
+              if (autoTransaction) {
+                //TODO: -- Oracle tr.commit() does not return a 'thenable' !? Ugly hack for now.
+                return (tr.commit(result) || Promise.resolve()).then(
+                  () => result
+                );
+              }
+
+              return result;
+            })
+            .catch((error) => {
+              if (autoTransaction) {
+                return tr.rollback(error).then(() => Promise.reject(error));
+              }
+
+              return Promise.reject(error);
+            });
+        })
+        .then(resolve)
+        .catch(reject);
+    }),
+    {
+      returning(columns) {
+        returning = columns;
+
+        return this;
+      },
+      transacting(tr) {
+        transaction = tr;
+
+        return this;
+      },
+    }
+  );
+
+  return wrapper;
+}

test/jake/migrate

@@ -1,94 +0,0 @@
-#!/usr/bin/env jake
-"use strict";
-
-const os = require('os');
-const fs = require('fs');
-const path = require('path');
-const sqlite3 = require('sqlite3');
-const assert = require('assert');
-
-const KNEX_PATH = path.normalize(__dirname + '/../../knex.js')
-const KNEX = path.normalize(__dirname + '/../../bin/cli.js')
-
-/* * * HELPERS * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
-
-function assertExec(cmd, desc) {
-    if (!desc) desc = 'Run '+cmd
-    return new Promise((resolve, reject)=> {
-        var stderr = '';
-        var bin = jake.createExec([`KNEX_PATH=${KNEX_PATH} ${cmd}`]);
-        bin.addListener('error', (msg, code)=> reject(Error(desc +' FAIL. '+ stderr)));
-        bin.addListener('cmdEnd', resolve);
-        bin.addListener('stderr', (data)=> stderr += data.toString());
-        bin.run();
-    });
-}
-
-var taskList = [];
-function test(description, func) {
-    var tempFolder, itFails=false;
-    tempFolder = fs.mkdtempSync(os.tmpdir() + '/knex-test-');
-    fs.mkdirSync(tempFolder + '/migrations');
-    desc(description);
-    let taskName = description.replace(/[^a-z0-9]/g, '');
-    taskList.push(taskName);
-    task(taskName, {async: true}, ()=> func(tempFolder)
-        .then(()=> console.log('☑ '+description) )
-        .catch((err)=> {
-            console.log('☒ '+err.message);
-            itFails = true;
-        })
-        .then(()=> {
-            jake.exec(`rm -r ${tempFolder}`);
-            if (itFails) process.exit(1);
-        })
-    );
-}
-
-
-/* * * TESTS * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
-
-test('Create a migration file', (temp)=>
-    assertExec(`${KNEX} migrate:make \
-               --client=sqlite3 \
-               --migrations-directory=${temp}/migrations \
-               create_rule_table`)
-    .then(()=>
-        assertExec(`ls ${temp}/migrations/*_create_rule_table.js`,
-                   'Find the migration file')
-    )
-    .then(()=>
-        assertExec(`grep exports.up ${temp}/migrations/*_create_rule_table.js`,
-                   'Migration created with boilerplate')
-    )
-);
-
-test('Run migrations', (temp)=>
-    (new Promise((resolve, reject)=>
-        fs.writeFile(temp+'/migrations/000_create_rule_table.js', `
-            exports.up = (knex)=> knex.schema.createTable('rules', (table)=> {
-                table.string('name');
-            });
-            exports.down = (knex)=> knex.schema.dropTable('rules');
-        `, (err)=> err ? reject(err) : resolve())
-    ))
-    .then(()=>
-        assertExec(`${KNEX} migrate:latest \
-                   --client=sqlite3 --connection=${temp}/db \
-                   --migrations-directory=${temp}/migrations \
-                   create_rule_table`)
-    )
-    .then(()=> assertExec(`ls ${temp}/db`, 'Find the database file') )
-    .then(()=> new sqlite3.Database(temp+'/db') )
-    .then((db)=>
-        new Promise((resolve, reject)=>
-            db.get(
-                'SELECT name FROM knex_migrations',
-                function(err, row){ err ? reject(err) : resolve(row) }
-            )
-        )
-    )
-    .then((row)=> assert.equal(row.name, '000_create_rule_table.js') )
-);
-
-task('default', taskList);

test/jake/migrate.js

@@ -0,0 +1,126 @@
+#!/usr/bin/env jake
+'use strict';
+/* eslint-disable no-undef */
+/* eslint-disable no-console */
+
+const os = require('os');
+const fs = require('fs');
+const rimrafSync = require('rimraf').sync;
+const path = require('path');
+const sqlite3 = require('sqlite3');
+const { assert } = require('chai');
+
+const KNEX_PATH = path.normalize(__dirname + '/../../knex.js');
+const KNEX = path.normalize(__dirname + '/../../bin/cli.js');
+
+/* * * HELPERS * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
+
+function assertExec(cmd, desc) {
+  desc = desc || 'Run ' + cmd;
+  return new Promise((resolve, reject) => {
+    let stderr = '';
+    const bin = jake.createExec([`KNEX_PATH=${KNEX_PATH} ${cmd}`]);
+    bin.addListener('error', (msg, code) =>
+      reject(Error(desc + ' FAIL. ' + stderr))
+    );
+    bin.addListener('cmdEnd', resolve);
+    bin.addListener('stderr', (data) => (stderr += data.toString()));
+    bin.run();
+  });
+}
+
+const taskList = [];
+function test(description, func) {
+  const tmpDirPath = os.tmpdir() + '/knex-test-';
+  let itFails = false;
+  rimrafSync(tmpDirPath);
+  const tempFolder = fs.mkdtempSync(tmpDirPath);
+  fs.mkdirSync(tempFolder + '/migrations');
+  desc(description);
+  const taskName = description.replace(/[^a-z0-9]/g, '');
+  taskList.push(taskName);
+  task(taskName, { async: true }, () =>
+    func(tempFolder)
+      .then(() => console.log('☑ ' + description))
+      .catch((err) => {
+        console.log('☒ ' + err.message);
+        itFails = true;
+      })
+      .then(() => {
+        jake.exec(`rm -r ${tempFolder}`);
+        if (itFails) {
+          process.exit(1);
+        }
+      })
+  );
+}
+
+/* * * TESTS * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
+
+test('Create a migration file', (temp) =>
+  assertExec(`${KNEX} migrate:make \
+               --client=sqlite3 \
+               --migrations-directory=${temp}/migrations \
+               create_rule_table`)
+    .then(() =>
+      assertExec(
+        `ls ${temp}/migrations/*_create_rule_table.js`,
+        'Find the migration file'
+      )
+    )
+    .then(() =>
+      assertExec(
+        `grep exports.up ${temp}/migrations/*_create_rule_table.js`,
+        'Migration created with boilerplate'
+      )
+    ));
+
+test('Create a migration file without client passed', (temp) =>
+  assertExec(`${KNEX} migrate:make \
+               --migrations-directory=${temp}/migrations \
+               create_rule_table`)
+    .then(() =>
+      assertExec(
+        `ls ${temp}/migrations/*_create_rule_table.js`,
+        'Find the migration file'
+      )
+    )
+    .then(() =>
+      assertExec(
+        `grep exports.up ${temp}/migrations/*_create_rule_table.js`,
+        'Migration created with boilerplate'
+      )
+    ));
+
+test('Run migrations', (temp) =>
+  new Promise((resolve, reject) =>
+    fs.writeFile(
+      temp + '/migrations/000_create_rule_table.js',
+      `
+            exports.up = (knex)=> knex.schema.createTable('rules', (table)=> {
+                table.string('name');
+            });
+            exports.down = (knex)=> knex.schema.dropTable('rules');
+        `,
+      (err) => (err ? reject(err) : resolve())
+    )
+  )
+    .then(() =>
+      assertExec(`${KNEX} migrate:latest \
+                   --client=sqlite3 --connection=${temp}/db \
+                   --migrations-directory=${temp}/migrations \
+                   create_rule_table`)
+    )
+    .then(() => assertExec(`ls ${temp}/db`, 'Find the database file'))
+    .then(() => new sqlite3.Database(temp + '/db'))
+    .then(
+      (db) =>
+        new Promise((resolve, reject) =>
+          db.get('SELECT name FROM knex_migrations', function(err, row) {
+            err ? reject(err) : resolve(row);
+          })
+        )
+    )
+    .then((row) => assert.equal(row.name, '000_create_rule_table.js')));
+
+task('default', taskList);