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

knex 0.8.0+ migration functions hang if not returning a promise #833

Closed
jurko-gospodnetic opened this Issue May 19, 2015 · 2 comments

Comments

Projects
None yet
2 participants
@jurko-gospodnetic
Collaborator

jurko-gospodnetic commented May 19, 2015

When porting our application to use knex 0.8.5 instead of knex 0.7.5 I encountered the following problem:

Previously when running a knex migration up()/down() functions, those functions could do nothing, i.e. return undefined and that would be treated the same as if we returned an already resolved promise.

This behaviour has been changed and now not returning a promise like that effectively leaves that migration step hanging indefinitely.

I can understand why the change has been made - you have two opposite use-cases:

  1. User performs an operation like knex('table').insert(...)... and forgets the return statement in front of it. Here, the new warning + a very visible migration hang helps as it points out the error to the user so the started async operation is not left running in the background as a daemon while the following migration steps are run and potentially causing random migration failures.
  2. Simple do-nothing migration steps. Here the change is a nuisance as such no-op functions now need to be changed to return an already resolved promise, or they risk causing failed/hung migrations.

Just wanted to confirm that the change was indeed intentional.

I got confused because example code generated by the src/migrate/stub/js.stub can still generate empty no-op up()/down() operations which would cause migration hangs/failures as mentioned above.

@bendrucker

This comment has been minimized.

Collaborator

bendrucker commented May 19, 2015

I doubt this was intentional. I would definitely advise not using empty migrator functions. In a future version of the migration API, an undefined return value will trigger an exception. In order to do a noop migration you'll have to return false. Guessing this has to do with the use of transactions for migrations since no promise is returned and the transaction has no signal for when to commit.

@tgriesser tgriesser closed this in 60ed217 May 20, 2015

@jurko-gospodnetic

This comment has been minimized.

Collaborator

jurko-gospodnetic commented May 20, 2015

thanks! reviewed the commit, looks great... :-)

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