Skip to content

Callback to getInstance not called #546

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

Closed
2 tasks
cellis opened this issue Feb 10, 2018 · 6 comments
Closed
2 tasks

Callback to getInstance not called #546

cellis opened this issue Feb 10, 2018 · 6 comments

Comments

@cellis
Copy link

cellis commented Feb 10, 2018

I'm submitting a...

  • [x ] Bug report
  • Feature request
  • Question

Current behavior

When I run the script yarn migrate up and there are no migrations I recieve:

$ babel-node ./internal/tools/migrate up
[INFO] No migrations to run
callback was called

However, if there are migrations or I run yarn migrate create some-migration, this happens:

$ babel-node ./internal/tools/migrate create some-migration
[INFO] Created migration at /migrations/20180210210707-some-migration.js
[INFO] Created migration up sql file at /migrations/sqls/20180210210707-some-migration-up.sql
[INFO] Created migration down sql file at /migrations/sqls/20180210210707-some-migration-down.sql
✨  Done in 2.19s.

Note that the callback was never called in the second example

My code is as follows:

require('db-migrate').getInstance(false, migrateConfig, () => {
  console.log('callback was called'); // never called
}).run();

Expected behavior

I expect a callback passed to getInstance to be called.

Minimal reproduction of the problem with instructions

I have a file called migrate.js with the above code inside. I'm executing it like this:

$ node migrate.js

What is the motivation / use case for changing the behavior?

I would like to be able to run some additional tasks after the database is migrated.

Environment


db-migrate-pg: "0.3.0",
plugins with versions: X.Y.Z
db-migrate driver with versions: 

Additional information:
- Node version: 9.2.0  
- Platform: MAC  

Others:

@wzrdtales
Copy link
Member

wzrdtales commented Feb 11, 2018

Not a bug but a wrong understanding of what you're passing. This is not a callback for any command you pass by in any sense and it is not intended to be in any case.

As the documentation states:

If you want to handle the onComplete callback in your own project you can do this like this:

This is about the onComplete callback, which is only called on actual migration actions and for nothing else. So you can't and shouldn't expect a create command to be called, when this is a cleanup callback to close connections and do several other stuff.

@wzrdtales
Copy link
Member

If you want to use db-migrate in a programmatic way, please use the actual programmatic options. Like

https://github.com/db-migrate/api-examples/blob/master/promise.js

@cellis
Copy link
Author

cellis commented Feb 11, 2018

@wzrdtales Then perhaps I should change this to a feature request. I have forked this in order to make run accept a callback. I don't need access to individual commands like create, up,down etc. I just want to execute something after the migration has run. If there's another way (e.g. with CLI) I'm all ears.

@wzrdtales
Copy link
Member

@cellis Not gonna happen, run is not intended for you to run, it is the non programmatic way and it is not going to be retouched for this.

And actually when you want to do something after your migration, why don't you just chain commands?

db-migrate up && echo 'hello this is the next command'

or the npm-way

  "scripts": {
    "migrate": "db-migrate up",
    "postmigrate": "echo hello"
  }

There is really absolutely no need if you just want to execute something after your migrations happened to use the programmatic mode, except if you plan to interact or integrate tightly. And then for your use case this is as well just the right way instead of run. Quoting you:

just want to execute something after the migration has run. If there's another way (e.g. with CLI) I'm all ears.

dbm.up() is exactly that and calling run instead is like buying a country when you want to buy a house.

@cellis
Copy link
Author

cellis commented Feb 11, 2018

...why don't you just chain commands?

What if i want to be able to run arbitrary commands, create, up,down, all with the same migrate commands. I also want to be able to specify an environment, which is why I use the programmatic API, because I can load my config. So, that means I would need:

"scripts": {
  "migrate:up": "db-migrate up && run next command",
  "migrate:down": "db-migrate down && run next command",
}

And that still doesn't exactly solve the problems. For instance, what happens if i want to run down -c 5? How do I pass the flags? So yes, I still want this as a feature.

@wzrdtales

@wzrdtales
Copy link
Member

Just as documented.

dbm.down(5) is the exact equivalent of -c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants