building5 / sails-db-migrate

db-migrate integration for sails.js
MIT License
61 stars 32 forks source link

Sails 0.12.3: Warning: a promise was created in a handler but was not returned from it #24

Closed ultrasaurus closed 8 years ago

ultrasaurus commented 8 years ago

I know your README reports that this supports Sails 0.10.x, but I've isolated a warning to this package (I think), so I figured I would report it for your consideration.

grunt db:migrate:up reports a warning about promises...

Running "db:migrate:up" (db:migrate) task
+ db-migrate up
DATABASE_URL=postgres://localhost/sailsdbtest
[INFO] Processed migration 20160410210432-add-pets
Warning: a promise was created in a handler but was not returned from it
    at Object.Base.extend.close (/Users/sarahallen/src/18F/sailsdbtest/node_modules/db-migrate-pg/index.js:477:26)
    at onComplete (/Users/sarahallen/src/18F/sailsdbtest/node_modules/db-migrate/api.js:857:19)
    at /Users/sarahallen/src/18F/sailsdbtest/node_modules/db-migrate/lib/migrator.js:172:11
From previous event:
    at /Users/sarahallen/src/18F/sailsdbtest/node_modules/db-migrate/lib/migrator.js:171:10
    at /Users/sarahallen/src/18F/sailsdbtest/node_modules/db-migrate/lib/migration.js:339:7
    at processImmediate [as _immediateCallback] (timers.js:383:17)

[INFO] Done

I've reproduced this in a very tiny Sails app and verified that if I use plain old db-migrate up from the node module (without sails-db-migrate) that I don't get the warning

Isolated Test Case

You can clone the simple app here -- The master branch just has the warning, the no-warn branch includes a database.json required to run the vanilla db-migrate

Steps:

nvm use 4.2.2
sails --version
0.12.3

sails new sailsdbtest
cd sailsdbtest
npm install sails-postgresql --save
npm install db-migrate --save-dev
npm install db-migrate-pg --save-dev
npm install sails-db-migrate --save-dev

echo "module.exports = require('sails-db-migrate').gruntTasks;" >> tasks/register/dbMigrate.js

createdb sailsdbtest

in config/migrations.js:

module.exports.migrations = {
  connection: 'postgresql'
};
grunt db:migrate:create --name add-pets
grunt db:migrate:up
leedm777 commented 8 years ago

@ultrasaurus Thanks for the detailed test case!

This is a bug in db-migrate's promise handling, and possibly Bluebird being a bit overzealous in trying to catch Promise related errors.

You can safely ignore this warning for now, and I've submitted a PR (db-migrate/node-db-migrate#367) to db-migrate to fix the promise handling in their code.

ultrasaurus commented 8 years ago

thanks for the quick response @leedm777 !