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

1.2.0: .nodeify doesn't return a promise #168

Closed
oleksiyk opened this Issue Mar 31, 2014 · 11 comments

Comments

Projects
None yet
4 participants
@oleksiyk

oleksiyk commented Mar 31, 2014

Hi!

I noticed .nodefiy doesn't return a promise back in 1.2.0:

var Promise = require('bluebird')

var cb = function () {}
var p = Promise.delay(500).nodeify(cb)

console.log(p) // --> undefined
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Mar 31, 2014

Owner

This is to prevent people from using callbacks and promises at the same time

Owner

petkaantonov commented Mar 31, 2014

This is to prevent people from using callbacks and promises at the same time

@oleksiyk

This comment has been minimized.

Show comment
Hide comment
@oleksiyk

oleksiyk Mar 31, 2014

Isn't it what documentation says here: https://github.com/petkaantonov/bluebird/blob/master/API.md#nodeifyfunction-callback---promise

Returns back this promise instead of creating a new one.

It is also a breaking change for projects that were exporting both interfaces. How is it possible that such breaking change is made without any acknowledgements in minor version upgrade?

oleksiyk commented Mar 31, 2014

Isn't it what documentation says here: https://github.com/petkaantonov/bluebird/blob/master/API.md#nodeifyfunction-callback---promise

Returns back this promise instead of creating a new one.

It is also a breaking change for projects that were exporting both interfaces. How is it possible that such breaking change is made without any acknowledgements in minor version upgrade?

@oleksiyk

This comment has been minimized.

Show comment
Hide comment
@oleksiyk

oleksiyk Mar 31, 2014

I see that it returns undefined only in the presence of callback, but it still breaks hundreds of tests people may had in their projects: checking for both valid promise and called callback. Now each test should be duplicated. You had only 1 'unintuitive' issue report now you have broken dependant projects.

oleksiyk commented Mar 31, 2014

I see that it returns undefined only in the presence of callback, but it still breaks hundreds of tests people may had in their projects: checking for both valid promise and called callback. Now each test should be duplicated. You had only 1 'unintuitive' issue report now you have broken dependant projects.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Mar 31, 2014

Collaborator

@oleksiyk #151

In a line - it didn't work too well before.

In my opinion - a dual interface is a mistake and opens up all sorts of problems, especially given how simple (and fast!) it is to promisify a node interface, or nodeify a promise based one. You can always support a promise interface and tell users who want callbacks to .nodeify at the end of their chain or support a callback interface and tell users to consider Bluebird and .promisifyAll

That said, I fully see why this change upsets you, we try to make as few such breaking changes as possible but sometimes we realize an API mistake was made. Sorry.

Collaborator

benjamingr commented Mar 31, 2014

@oleksiyk #151

In a line - it didn't work too well before.

In my opinion - a dual interface is a mistake and opens up all sorts of problems, especially given how simple (and fast!) it is to promisify a node interface, or nodeify a promise based one. You can always support a promise interface and tell users who want callbacks to .nodeify at the end of their chain or support a callback interface and tell users to consider Bluebird and .promisifyAll

That said, I fully see why this change upsets you, we try to make as few such breaking changes as possible but sometimes we realize an API mistake was made. Sorry.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Mar 31, 2014

Owner

I think we can quickly fix this in a patch.. 2.0 will come soon anyway

Owner

petkaantonov commented Mar 31, 2014

I think we can quickly fix this in a patch.. 2.0 will come soon anyway

@oleksiyk

This comment has been minimized.

Show comment
Hide comment
@oleksiyk

oleksiyk Mar 31, 2014

Yeah, sorry I was too much upset. I agree that this is correct behaviour. That's just that I need to fix about 1000 tests in my projects.

oleksiyk commented Mar 31, 2014

Yeah, sorry I was too much upset. I agree that this is correct behaviour. That's just that I need to fix about 1000 tests in my projects.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Mar 31, 2014

Owner

Yes I did not realize anyone was actually relying on this behavior - I take backwards compatibility very seriously so I will fix this asap in a patch.

Owner

petkaantonov commented Mar 31, 2014

Yes I did not realize anyone was actually relying on this behavior - I take backwards compatibility very seriously so I will fix this asap in a patch.

@oleksiyk

This comment has been minimized.

Show comment
Hide comment
@oleksiyk

oleksiyk Mar 31, 2014

I actually think that both interfaces can only be used in such limited cases as tests (just to save typing and copy/pasting). So - you decide.

oleksiyk commented Mar 31, 2014

I actually think that both interfaces can only be used in such limited cases as tests (just to save typing and copy/pasting). So - you decide.

petkaantonov added a commit that referenced this issue Mar 31, 2014

@jmdobry

This comment has been minimized.

Show comment
Hide comment
@jmdobry

jmdobry Apr 11, 2014

@petkaantonov
A library of mine (using Bluebird of course) exposes the dual callback-promise interface. What does this issue mean for my library's continued use of Bluebird?

jmdobry commented Apr 11, 2014

@petkaantonov
A library of mine (using Bluebird of course) exposes the dual callback-promise interface. What does this issue mean for my library's continued use of Bluebird?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Apr 12, 2014

Owner

It makes no sense to use both interfaces simultaneously so probably nothing

Owner

petkaantonov commented Apr 12, 2014

It makes no sense to use both interfaces simultaneously so probably nothing

@jmdobry

This comment has been minimized.

Show comment
Hide comment
@jmdobry

jmdobry Apr 12, 2014

So, I can still export both interfaces, but only one can be used, right?
On Apr 12, 2014 2:34 AM, "Petka Antonov" notifications@github.com wrote:

It makes no sense to use both interfaces simultaneously so probably nothing

Reply to this email directly or view it on GitHubhttps://github.com/petkaantonov/bluebird/issues/168#issuecomment-40275174
.

jmdobry commented Apr 12, 2014

So, I can still export both interfaces, but only one can be used, right?
On Apr 12, 2014 2:34 AM, "Petka Antonov" notifications@github.com wrote:

It makes no sense to use both interfaces simultaneously so probably nothing

Reply to this email directly or view it on GitHubhttps://github.com/petkaantonov/bluebird/issues/168#issuecomment-40275174
.

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