throw error if a migration method doesn't return a thenable

[jedwards1211]

Although the README says that

In order to allow asynchronicity, tasks have return a Promise object which provides a then method

In actuality, tasks that don't return a Promise succeed and can cause intermittent failures if something else is writing to the database at the same time (since umzug can't wait for the operations in the task to finish). I just discovered yesterday that I wasn't returning Promises from my migration tasks (because I hadn't RTFM...), and now I know why I had seen inconsistent migration behavior in the past.

This PR at least throws an error whenever a task doesn't return a thenable object, so that devs will find out immediately if they make this mistake.

[PascalPflaum]

I agree, that sometimes guiding a user a little bit is nice, especially if it can have strange race conditions like here. Still this is changing the behaviour of a existing version, so will include it in the next minor release.

It looks good to me, I will merge it, when I have some spare time.

Thanks for contributing!

[jedwards1211]

@PascalPflaum I think according to semver this probably qualifies as a breaking change, even though not returning promise is incorrect usage of umzug.

[PascalPflaum]

I thought about that as well, but I will thread it as minor. In my eyes it is not a breaking change because not returning a promise leads to errors and is against the documentation. It results in errors before and will result in errors afterwars, but a verbose error, not based on race conditions.

The only alternative I see is to add a deprecation warning in 2.2.x and throw a error in 3.x.

[jedwards1211]

Yeah, that's a good point. Some people may have to make code changes but it will help them unbreak what was already broken.

[jedwards1211]

@PascalPflaum hey I noticed that the PR failure was due to test timeouts unrelated to my code changes:

  2 failing
  1) custom resolver can resolve sql files:
     Error: Timeout of 1000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
  
  2) custom resolver can resolve typescript files:
     SequelizeDatabaseError: SQLITE_IOERR: disk I/O error
      at Query.module.exports.Query.formatError (node_modules/sequelize/lib/dialects/sqlite/query.js:239:16)
      at Statement.<anonymous> (node_modules/sequelize/lib/dialects/sqlite/query.js:47:31)
      at Statement.replacement (node_modules/sqlite3/lib/trace.js:19:31)
      at Statement.replacement (node_modules/sqlite3/lib/trace.js:19:31)