Skip to content
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

The framework does not recognize a Bluebird Promise #84

Closed
XVincentX opened this issue Nov 4, 2017 · 8 comments
Closed

The framework does not recognize a Bluebird Promise #84

XVincentX opened this issue Nov 4, 2017 · 8 comments

Comments

@XVincentX
Copy link
Contributor

Hey — I've been this library for some time (in particular the next version) and so far it has been working great, thanks.

Now, I'm in a migration where for some reasons the promises being returned from the code aren't native from Node, but rather from Bluebird.

Therefore node-migrate does not recognize them (as they're not instanceof Promise) and switches back to callbacks, that aren't provided of course.

It would be good if it could work with Bluebird promises as well and more importantly, if no async mechanism is detected, a warning could be shown.

Thanks!

@wesleytodd
Copy link
Collaborator

With native Promise support so prevalent I do not thing this lib should support any third-party implementations. Are the migrations not something you can convert to native promises?

@XVincentX
Copy link
Contributor Author

I totally agree with you. I didn't have the time to look in a way to convert bluebird promises into a native promise…probably:

return new Promise((resolve, reject)=>bluebird.then(resolve).catch(reject)) 

Anyway I was wondering whether a simple check if then function exist wouldn't be enough…

@wesleytodd
Copy link
Collaborator

I think your wrapper would be right, although I haven't ever use bluebird directly.

The check for a then function is a bit naive for my taste. If someone has some ORM or db driver which has a then method and accidentally returns it, it would trigger this.

@XVincentX
Copy link
Contributor Author

XVincentX commented Nov 5, 2017

It definitely make sense. I'll try to figure it out somehow.
Anyway — as I've spent hours on trying to debug the issue before realizing the real problem — I think it would be really beneficial to at least show a warning if no async mechanism is found (such as No Promise and no callback function as last parameter)

@XVincentX XVincentX reopened this Nov 5, 2017
@wesleytodd
Copy link
Collaborator

Yep! Thanks for the input, sorry I couldn't help more. Let me know if you have any other suggestions, I have been waiting to publish the 1.0.0 until I got some feedback that people have tested it a bit, so thanks for doing that!

@XVincentX
Copy link
Contributor Author

it would be really beneficial to at least show a warning if no async mechanism is found (such as No Promise and no callback function as last parameter)

@wesleytodd
Copy link
Collaborator

Good point. I will take a look at that before I publish, probably later this weekend.

@wesleytodd
Copy link
Collaborator

I am going to close this in favor of #87.

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

No branches or pull requests

2 participants