-
Notifications
You must be signed in to change notification settings - Fork 225
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
Implements support for promise based migrations. #82
Changes from 1 commit
ef643ac
0a39d4a
8ca6807
ffc5300
7b453fe
22abdb0
21e521a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,9 +26,14 @@ function migrate (set, direction, migrationName, fn) { | |
} | ||
|
||
set.emit('migration', migration, direction) | ||
migration[direction](function (err) { | ||
if (err) return fn(err) | ||
let migrationCompleted = false | ||
|
||
let migrationPromise = migration[direction](function (err) { | ||
if (migrationCompleted) { | ||
return fn(new Error('Migration was already run. Please provide callback OR promise, not both.')) | ||
} | ||
if (err) return fn(err) | ||
migrationCompleted = true | ||
// Set timestamp if running up, clear it if down | ||
migration.timestamp = direction === 'up' ? Date.now() : null | ||
|
||
|
@@ -42,6 +47,30 @@ function migrate (set, direction, migrationName, fn) { | |
next(migrations.shift()) | ||
}) | ||
}) | ||
|
||
if (migrationPromise && migrationPromise.then) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer this to be |
||
migrationPromise | ||
.then(function () { | ||
if (migrationCompleted) { | ||
return fn(new Error('Migration was already run. Please provide callback OR promise, not both.')) | ||
} | ||
|
||
migrationCompleted = true | ||
// Set timestamp if running up, clear it if down | ||
migration.timestamp = direction === 'up' ? Date.now() : null | ||
|
||
// Decrement last run index | ||
lastRunIndex-- | ||
|
||
set.lastRun = direction === 'up' ? migration.title : set.migrations[lastRunIndex] && set.migrations[lastRunIndex].title | ||
set.save(function (err) { | ||
if (err) return fn(err) | ||
|
||
next(migrations.shift()) | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this fine, or should i split the callback out into a separate function, which can be reused in both callback & promise cases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I would do that for sure. |
||
}) | ||
.catch(fn) | ||
} | ||
} | ||
|
||
next(migrations.shift()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
'use strict' | ||
|
||
module.exports.up = function (next) { | ||
setTimeout(function () { | ||
return next() | ||
}, 200) | ||
} | ||
|
||
module.exports.down = function (next) { | ||
setTimeout(function () { | ||
return next() | ||
}, 200) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To reduce test time, just do a timeout of |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
'use strict' | ||
|
||
module.exports.up = function () { | ||
return new Promise(function (resolve, reject) { | ||
setTimeout(function () { | ||
return resolve() | ||
}, 200) | ||
}) | ||
} | ||
|
||
module.exports.down = function () { | ||
return new Promise(function (resolve, reject) { | ||
setTimeout(function () { | ||
return resolve() | ||
}, 200) | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
'use strict' | ||
|
||
module.exports.up = function (next) { | ||
return new Promise(function (resolve, reject) { | ||
setTimeout(function () { | ||
next() | ||
return resolve() | ||
}, 200) | ||
}) | ||
} | ||
|
||
module.exports.down = function (next) { | ||
return new Promise(function (resolve, reject) { | ||
setTimeout(function () { | ||
next() | ||
return resolve() | ||
}, 200) | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
/* global describe, it, beforeEach, afterEach */ | ||
|
||
var rimraf = require('rimraf') | ||
var path = require('path') | ||
var assert = require('assert') | ||
|
||
var migrate = require('../') | ||
|
||
var BASE = path.join(__dirname, 'fixtures', 'promises') | ||
var STATE = path.join(__dirname, 'fixtures', '.migrate') | ||
|
||
describe('Promise migrations', function () { | ||
var set | ||
|
||
beforeEach(function (done) { | ||
migrate.load({ | ||
stateStore: STATE, | ||
migrationsDirectory: BASE | ||
}, function (err, s) { | ||
set = s | ||
done(err) | ||
}) | ||
}) | ||
|
||
it('should handle callback migration', function (done) { | ||
set.up('1-callback-test.js', function (err) { | ||
assert.ifError(err) | ||
done() | ||
}) | ||
}) | ||
|
||
it('should handle promise migration', function (done) { | ||
set.up('2-promise-test.js', function (err) { | ||
assert.ifError(err) | ||
done() | ||
}) | ||
}) | ||
|
||
it('should error when using promise + callback', function (done) { | ||
let errorThrown = false | ||
set.up('3-failure-test.js', function (err) { | ||
if (errorThrown) { | ||
return | ||
} | ||
|
||
assert(err) | ||
assert.equal(err.message, 'Migration was already run. Please provide callback OR promise, not both.') | ||
errorThrown = true | ||
done() | ||
}) | ||
}) | ||
|
||
afterEach(function (done) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this to the top like in the other tests. If this file grows it will get lost way down here. |
||
rimraf(STATE, done) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though I prefer multi-line if statements with curlys like this, the majority of the code base is done on a single line. Please follow the common formatting.