/bluebird

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

Promise.coroutine: yielding a function should not call the function #1314

Closed
giltayar opened this Issue Jan 4, 2017 · 4 comments

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@giltayar @benjamingr @petkaantonov
@giltayar
Contributor

giltayar commented Jan 4, 2017

  1. What version of bluebird is the issue happening on?
    3.4.7

  2. What platform and version? (For example Node.js 0.12 or Google Chrome 32)
    Tested on Node 7, but I'm guessing it's true for all platforms

  3. Did this issue happen with earlier version of bluebird?
    Probably.

Given:

(Promise.coroutine(function*() {
  yield function () { console.log('hi'); };
}))();

You would not expect "hi" to be written to log, because we're yielding a value (which is a function), and not calling the function.

But it does print "hi". And then returns a rejected error.

I have prepared a pull-request that contains a fix, which I will submit in a few minutes.

giltayar pushed a commit to giltayar/bluebird that referenced this issue Jan 4, 2017

Gil Tayar
Fixed #1314: yielding a function should not call the function 
   When rejecting a non-promise value, string.replace is used,
    and the value is passed as second parameter.
    But string.replace, if the second value is a function, will
    call that function.
    This commit fixes this by using value.toString().
c7f55fb
@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 4, 2017

Collaborator

Fix LGTM. @petkaantonov ptal.
Collaborator

benjamingr commented Jan 4, 2017

Fix LGTM. @petkaantonov ptal.
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jan 4, 2017

Owner

+ "" is sufficient
Owner

petkaantonov commented Jan 4, 2017

+ "" is sufficient
@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 4, 2017

Collaborator

Or String(value)
Collaborator

benjamingr commented Jan 4, 2017

Or String(value)
@giltayar

This comment has been minimized.

Show comment
Hide comment
@giltayar

giltayar Jan 5, 2017

Contributor

I chose @benjamingr's change - it seems more explicit to me. Thanks for pointing the (I hope slightly) awkward code out.
Contributor

giltayar commented Jan 5, 2017

I chose @benjamingr's change - it seems more explicit to me. Thanks for pointing the (I hope slightly) awkward code out.

@benjamingr benjamingr closed this in 0566325 Jan 27, 2017

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