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

Improve Warning: a promise was created in a handler but none were returned from it #1205

Closed
bergus opened this issue Aug 24, 2016 · 6 comments
Closed

Comments

@bergus
Copy link
Contributor

@bergus bergus commented Aug 24, 2016

It seems this warning is happening a lot and confusing quite a few users (see #508, #854, #1054, #1084 and many others).

Here are some ideas how the message could be improved:

  • include a link to http://bluebirdjs.com/docs/warning-explanations.html (maybe anchored to some id shorter than #warning-a-promise-was-created-in-a-handler-but-was-not-returned-from-it)
  • make the location of the handler function more prominent. An unsuspecting user should be pointed right to the line where the return is missing, the rest of the stack trace is not so important. I bet 80 to 90 percent of cases are "Right I forgot the return here", and only the minority is "but that function doesn't seem to return a promise, why would I need to return?" or "but I do return the result of the function, why is it not a promise?" that would make it necessary to look at the stack trace.
  • the stack trace itself should include the lines "promise created by new Promise/then/whatever" and "then/catch/whatever calls the handler"
@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Aug 24, 2016

I think we can fix #1 (PR #1207 created) - about the other two:

  • I think the stack trace is crucial since the users might return a promise conditionally (that is often the case) so I disagree with #2.
  • The warning message itself already includes the "promise created by " + name, feel free to improve on it.
@bergus
Copy link
Contributor Author

@bergus bergus commented Aug 24, 2016

By 2) I didn't mean that the stack trace should be omitted, rather that the line should be repeated.

For example:

function a() { new Promise(()=>{}); }
function b() { return a(); }
function d() {
    return x().then(function c(res) {
         res += ;
         b();
     })
}
d()

Now I'd love to get

Warning: a promise was created in the handler at test.js line 6
but was not returned from it, see http://goo.gl/rRqMUw
    at new Promise (bluebird.js)
    at a (test.js:1)
    at b (test.js:2)
    at c (test.js:6)
    at callThenHandler (bluebird.js)
from previous event:
    at d (test.js:4)
    at global (test.js:9)
@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Aug 24, 2016

The merged PR only added the link, new PR welcome that adds the line too

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Aug 25, 2016

Addressed both additional requirements @bergus.

"use strict";

var Promise = require("./js/debug/bluebird");

Promise.config({warnings: true})

function x() {
    return Promise.delay(3);
}
function a() { Promise.delay() }
function b() { return a(); }
function d() {
    return x().then(function c(res) {
         res += 3;
         b();
     }).then(function(){})
}
d()

Now gives:

Warning: a promise was created in a handler at /home/petka/bluebird/throwaway.js:15:10 but was not returned from it, see http://goo.gl/rRqMUw
    at Function.Promise.delay (/home/petka/bluebird/js/debug/timers.js:30:13)
    at a (/home/petka/bluebird/throwaway.js:10:24)
    at b (/home/petka/bluebird/throwaway.js:11:23)
    at c (/home/petka/bluebird/throwaway.js:15:10)
    at tryOnTimeout (timers.js:232:11)
    at Timer.listOnTimeout (timers.js:202:5)
From previous event:
    at d (/home/petka/bluebird/throwaway.js:13:16)
    at Object.<anonymous> (/home/petka/bluebird/throwaway.js:18:1)
    at Module._compile (module.js:556:32)
    at Object.Module._extensions..js (module.js:565:10)
    at Module.load (module.js:473:32)
    at tryModuleLoad (module.js:432:12)
    at Function.Module._load (module.js:424:3)
    at Module.runMain (module.js:590:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3

"use strict";

var Promise = require("./js/debug/bluebird");

Promise.config({warnings: true})

function x() {
    return Promise.delay(3);
}
function a() { new Promise(function() {}) }
function b() { return a(); }
function d() {
    return x().then(function c(res) {
         res += 3;
         b();
     }).then(function(){})
}
d()

Now gives:

(node:13673) Warning: a promise was created in a handler at /home/petka/bluebird/throwaway.js:15:10 but was not returned from it, see http://goo.gl/rRqMUw
    at new Promise (/home/petka/bluebird/js/debug/promise.js:78:14)
    at a (/home/petka/bluebird/throwaway.js:10:16)
    at b (/home/petka/bluebird/throwaway.js:11:23)
    at c (/home/petka/bluebird/throwaway.js:15:10)
    at tryOnTimeout (timers.js:232:11)
    at Timer.listOnTimeout (timers.js:202:5)
From previous event:
    at d (/home/petka/bluebird/throwaway.js:13:16)
    at Object.<anonymous> (/home/petka/bluebird/throwaway.js:18:1)
    at Module._compile (module.js:556:32)
    at Object.Module._extensions..js (module.js:565:10)
    at Module.load (module.js:473:32)
    at tryModuleLoad (module.js:432:12)
    at Function.Module._load (module.js:424:3)
    at Module.runMain (module.js:590:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Aug 25, 2016

Released in 3.4.3

Also, in node v6 without --trace-warnings the warning is now quite useful even without stack:

(node:14142) Warning: a promise was created in a handler at /home/petka/bluebird/throwaway.js:15:10 but was not returned from it, see http://goo.gl/rRqMUw
    at new Promise (/home/petka/bluebird/js/debug/promise.js:78:14)
@bergus
Copy link
Contributor Author

@bergus bergus commented Aug 25, 2016

Gorgeous, thank you!

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.