Implements support for promise based migrations. #82
Conversation
lib/migrate.js
Outdated
| if (err) return fn(err) | ||
|
|
||
| next(migrations.shift()) | ||
| }) |
There was a problem hiding this comment.
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.
Yeah, I would do that for sure.
lib/migrate.js
Outdated
| @@ -42,6 +47,30 @@ function migrate (set, direction, migrationName, fn) { | |||
| next(migrations.shift()) | |||
| }) | |||
| }) | |||
|
|
|||
| if (migrationPromise && migrationPromise.then) { | |||
There was a problem hiding this comment.
Prefer this to be typeof Promise !== 'undefined' && migrationPromise instanceof Promise
|
Would prefer to keep as much backward support as possible. Can you revert the Reading this reminded me to go back and ensure I did not use any arrow functions or |
| module.exports.down = function (next) { | ||
| setTimeout(function () { | ||
| return next() | ||
| }, 200) |
There was a problem hiding this comment.
To reduce test time, just do a timeout of 1 ms. It achieves the same effect without the wait.
test/promises.js
Outdated
| }) | ||
| }) | ||
|
|
||
| afterEach(function (done) { |
There was a problem hiding this comment.
Move this to the top like in the other tests. If this file grows it will get lost way down here.
lib/migrate.js
Outdated
|
|
||
| let migrationPromise = migration[direction](function (err) { | ||
| if (migrationCompleted) { |
There was a problem hiding this comment.
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.
|
@jdpnielsen Awesome work. Thanks! With these few changes I can merge this up before we do the |
|
Done. |
|
Weird, I amended the commit and added the |
No description provided.