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(bluebird): remove Bluebird.bind #3477

Merged
merged 4 commits into from Oct 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
123 changes: 52 additions & 71 deletions lib/dialects/sqlite3/schema/ddl.js
Expand Up @@ -4,7 +4,6 @@
// columns and changing datatypes.
// -------

const Bluebird = require('bluebird');
const {
assign,
uniqueId,
Expand All @@ -16,7 +15,8 @@ const {
fromPairs,
some,
negate,
isEmpty
isEmpty,
chunk,
} = require('lodash');

// So altering the schema in SQLite3 is a major pain.
Expand Down Expand Up @@ -83,52 +83,34 @@ assign(SQLite3_DDL.prototype, {
copyData() {
return this.trx
.raw(`SELECT * FROM "${this.tableName()}"`)
.bind(this)
.then(this.insertChunked(20, this.alteredName));
.then((result) =>
this.insertChunked(20, this.alteredName, identity, result)
);
},

reinsertData(iterator) {
return function() {
return this.trx
.raw(`SELECT * FROM "${this.alteredName}"`)
.bind(this)
.then(this.insertChunked(20, this.tableName(), iterator));
};
return this.trx
.raw(`SELECT * FROM "${this.alteredName}"`)
.then((result) =>
this.insertChunked(20, this.tableName(), iterator, result)
);
},

insertChunked(amount, target, iterator) {
async insertChunked(chunkSize, target, iterator, result) {
iterator = iterator || identity;
return function(result) {
let batch = [];
const ddl = this;
return Bluebird.reduce(
result,
function(memo, row) {
memo++;
batch.push(row);
if (memo % 20 === 0 || memo === result.length) {
return ddl.trx
.queryBuilder()
.table(target)
.insert(map(batch, iterator))
.then(function() {
batch = [];
})
.then(() => memo);
}
return memo;
},
0
);
};
const chunked = chunk(result, chunkSize);
for (const batch of chunked) {
await this.trx
.queryBuilder()
.table(target)
.insert(map(batch, iterator));
}
},

createTempTable(createTable) {
return function() {
return this.trx.raw(
createTable.sql.replace(this.tableName(), this.alteredName)
);
};
return this.trx.raw(
createTable.sql.replace(this.tableName(), this.alteredName)
);
},

_doReplace(sql, from, to) {
Expand Down Expand Up @@ -172,7 +154,7 @@ assign(SQLite3_DDL.prototype, {

const fromIdentifier = from.replace(/[`"'[\]]/g, '');

args = args.map(function(item) {
args = args.map((item) => {
let split = item.trim().split(' ');

// SQLite supports all quoting mechanisms prevalent in all major dialects of SQL
Expand All @@ -185,7 +167,7 @@ assign(SQLite3_DDL.prototype, {
new RegExp(`\`${fromIdentifier}\``, 'i'),
new RegExp(`"${fromIdentifier}"`, 'i'),
new RegExp(`'${fromIdentifier}'`, 'i'),
new RegExp(`\\[${fromIdentifier}\\]`, 'i')
new RegExp(`\\[${fromIdentifier}\\]`, 'i'),
];
if (fromIdentifier.match(/^\S+$/)) {
fromMatchCandidates.push(new RegExp(`\\b${fromIdentifier}\\b`, 'i'));
Expand All @@ -195,7 +177,10 @@ assign(SQLite3_DDL.prototype, {
some(fromMatchCandidates, (c) => target.match(c));

const replaceFromIdentifier = (target) =>
fromMatchCandidates.reduce((result, candidate) => result.replace(candidate, to), target);
fromMatchCandidates.reduce(
(result, candidate) => result.replace(candidate, to),
target
);

if (doesMatchFromIdentifier(split[0])) {
// column definition
Expand All @@ -213,7 +198,7 @@ assign(SQLite3_DDL.prototype, {
// columns from this table listed between (); replace
// one if it matches
if (/primary|unique/i.test(split[idx])) {
const ret = item.replace(/\(.*\)/, replaceFromIdentifier);
const ret = item.replace(/\(.*\)/, replaceFromIdentifier);
// If any member columns are dropped then uniqueness/pk constraint
// can not be retained
if (ret !== item && isEmpty(to)) return '';
Expand All @@ -240,7 +225,10 @@ assign(SQLite3_DDL.prototype, {

if (split[1].slice(0, tableName.length) === tableName) {
// self-referential foreign key
const replacedKeyTargetSpec = split[1].replace(/\(.*\)/, replaceFromIdentifier);
const replacedKeyTargetSpec = split[1].replace(
/\(.*\)/,
replaceFromIdentifier
);
if (split[1] !== replacedKeyTargetSpec) {
// If we are removing one or more columns of a foreign
// key, then we should not retain the key at all
Expand Down Expand Up @@ -289,20 +277,10 @@ assign(SQLite3_DDL.prototype, {
)
);

return Bluebird.bind(this)
.then(this.createTempTable(createTable))
.then(this.copyData)
.then(this.dropOriginal)
.then(function() {
return this.trx.raw(newSql);
})
.then(
this.reinsertData(function(row) {
row[mappedTo] = row[mappedFrom];
return omit(row, mappedFrom);
})
)
.then(this.dropTempTable);
return this.reinsertMapped(createTable, newSql, (row) => {
row[mappedTo] = row[mappedFrom];
return omit(row, mappedFrom);
});
},
{ connection: this.connection }
);
Expand All @@ -312,10 +290,9 @@ assign(SQLite3_DDL.prototype, {
return this.client.transaction(
(trx) => {
this.trx = trx;
return Bluebird.all(columns.map((column) => this.getColumn(column)))
.bind(this)
.then(this.getTableSql)
.then(function(sql) {
return Promise.all(columns.map((column) => this.getColumn(column)))
.then(() => this.getTableSql())
.then((sql) => {
const createTable = sql[0];
let newSql = createTable.sql;
columns.forEach((column) => {
Expand All @@ -330,20 +307,24 @@ assign(SQLite3_DDL.prototype, {
fromPairs(columns.map((column) => [column, column]))
)
);
return Bluebird.bind(this)
.then(this.createTempTable(createTable))
.then(this.copyData)
.then(this.dropOriginal)
.then(function() {
return this.trx.raw(newSql);
})
.then(this.reinsertData((row) => omit(row, ...mappedColumns)))
.then(this.dropTempTable);
return this.reinsertMapped(createTable, newSql, (row) =>
omit(row, ...mappedColumns)
);
});
},
{ connection: this.connection }
);
},

reinsertMapped(createTable, newSql, mapRow) {
return Promise.resolve()
.then(() => this.createTempTable(createTable))
.then(() => this.copyData())
.then(() => this.dropOriginal())
.then(() => this.trx.raw(newSql))
.then(() => this.reinsertData(mapRow))
.then(() => this.dropTempTable());
},
});

module.exports = SQLite3_DDL;
3 changes: 1 addition & 2 deletions lib/migrate/Migrator.js
@@ -1,6 +1,5 @@
// Migrator
// -------
const Bluebird = require('bluebird');
const {
differenceWith,
each,
Expand Down Expand Up @@ -486,7 +485,7 @@ class Migrator {
_waterfallBatch(batchNo, migrations, direction, trx) {
const trxOrKnex = trx || this.knex;
const { tableName, schemaName, disableTransactions } = this.config;
let current = Bluebird.bind({ failed: false, failedOn: 0 });
let current = Promise.resolve();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are those fields not used anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't found any usages in project

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7a6937f first revision where failedOn exists, i think it just partially implemented feature

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, guess we can drop it.

const log = [];
each(migrations, (migration) => {
const name = this.config.migrationSource.getMigrationName(migration);
Expand Down
21 changes: 10 additions & 11 deletions lib/runner.js
Expand Up @@ -219,17 +219,16 @@ Object.assign(Runner.prototype, {

// In the case of the "schema builder" we call `queryArray`, which runs each
// of the queries in sequence.
queryArray(queries) {
return queries.length === 1
? this.query(queries[0])
: Bluebird.bind(this)
.return(queries)
.reduce(function(memo, query) {
return this.query(query).then(function(resp) {
memo.push(resp);
return memo;
});
}, []);
async queryArray(queries) {
if (queries.length === 1) {
return this.query(queries[0]);
}

const results = [];
for (const query of queries) {
results.push(await this.query(query));
}
return results;
},

// Check whether there's a transaction flag, and that it has a connection.
Expand Down
88 changes: 27 additions & 61 deletions lib/seed/Seeder.js
Expand Up @@ -5,16 +5,7 @@ const fs = require('fs');
const path = require('path');
const { promisify } = require('util');
const mkdirp = require('mkdirp');
const Bluebird = require('bluebird');
const {
filter,
includes,
map,
bind,
template,
each,
extend,
} = require('lodash');
const { filter, includes, extend } = require('lodash');
const { writeJsFileUsingTemplate } = require('../util/template');

// The new seeds we're performing, typically called from the `knex.seed`
Expand All @@ -29,7 +20,7 @@ class Seeder {
// Runs seed files for the given environment.
async run(config) {
this.config = this.setConfig(config);
const [all] = await this._seedData();
const all = await this._listAll();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will types changed in https://github.com/knex/knex/pull/3438/files still be correct after this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_seedData is just _listAll, but wrapping result inside promise into []

const files =
config && config.specific
? all.filter((file) => file === config.specific)
Expand Down Expand Up @@ -62,11 +53,6 @@ class Seeder {
);
}

// Gets the seed file list from the specified seed directory.
_seedData() {
return Bluebird.join(this._listAll());
}

// Ensures a folder for the seeds exist, dependent on the
// seed config settings.
async _ensureFolder() {
Expand All @@ -80,13 +66,8 @@ class Seeder {

// Run seed files, in sequence.
_runSeeds(seeds) {
return Bluebird.all(map(seeds, bind(this._validateSeedStructure, this)))
.bind(this)
.then(function(seeds) {
return Bluebird.bind(this).then(function() {
return this._waterfallBatch(seeds);
});
});
seeds.forEach((seed) => this._validateSeedStructure(seed));
return this._waterfallBatch(seeds);
}

// Validates seed files by requiring and checking for a `seed` function.
Expand All @@ -105,14 +86,6 @@ class Seeder {
);
}

// Generates the stub template for the current seed file, returning a compiled template.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorefnon I assume we stopped using it after your refactoring?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i moved the lodash template usage to a shared utility which we can potentially use from other places as well.

_generateStubTemplate() {
const stubPath = this._getStubPath();
return promisify(fs.readFile)(stubPath).then(
(stub) => template(stub.toString(), { variable: 'd' })
);
}

_getNewStubFileName(name) {
if (name[0] === '-') name = name.slice(1);
return name + '.' + this.config.extension;
Expand All @@ -136,39 +109,32 @@ class Seeder {
}

// Runs a batch of seed files.
_waterfallBatch(seeds) {
async _waterfallBatch(seeds) {
const { knex } = this;
const seedDirectory = this._absoluteConfigDir();
let current = Bluebird.bind({ failed: false, failedOn: 0 });
const log = [];
each(seeds, (seed) => {
const name = path.join(seedDirectory, seed);
seed = require(name);

// Run each seed file.
current = current.then(() =>
// Nesting promise to prevent bubbling up of error on catch
Promise.resolve()
.then(() => seed.seed(knex))
.then(() => log.push(name))
.catch((originalError) => {
const error = new Error(
`Error while executing "${name}" seed: ${originalError.message}`
);
error.original = originalError;
error.stack =
error.stack
.split('\n')
.slice(0, 2)
.join('\n') +
'\n' +
originalError.stack;
throw error;
})
);
});

return current.then(() => [log]);
for (const seedName of seeds) {
const seedPath = path.join(seedDirectory, seedName);
const seed = require(seedPath);
try {
await seed.seed(knex);
log.push(seedPath);
} catch (originalError) {
const error = new Error(
`Error while executing "${seedPath}" seed: ${originalError.message}`
);
error.original = originalError;
error.stack =
error.stack
.split('\n')
.slice(0, 2)
.join('\n') +
'\n' +
originalError.stack;
throw error;
}
}
return [log];
}

_absoluteConfigDir() {
Expand Down