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

No stack trace provided if a rejected Promise's .done() method is called #411

Closed
nomcopter opened this Issue Dec 16, 2014 · 17 comments

Comments

Projects
None yet
3 participants
@nomcopter

nomcopter commented Dec 16, 2014

When Promise.reject(new Error('error')) is ran, a Possibly unhandled Error... is thrown in the browser with a very useful stack trace.

However, when you .done() the promise, no useful stack trace is produced.

screen shot 2014-12-16 at 1 36 12 pm

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Dec 16, 2014

Collaborator

How is that stack trace useful in any way o_0?

Collaborator

benjamingr commented Dec 16, 2014

How is that stack trace useful in any way o_0?

@nomcopter

This comment has been minimized.

Show comment
Hide comment
@nomcopter

nomcopter Dec 16, 2014

It's the stack trace of what is running in the console, it is much more useful when it is actually running in code instead :)

The Object.InjectedScript is part of Chrome's internal console code, the anonymous function is what I typed into the console.

nomcopter commented Dec 16, 2014

It's the stack trace of what is running in the console, it is much more useful when it is actually running in code instead :)

The Object.InjectedScript is part of Chrome's internal console code, the anonymous function is what I typed into the console.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Dec 16, 2014

Collaborator

Yeah, I understand what stack trace I'm seeing, what I don't understand is why you find it useful in this case - now if you had Promise.longStackTraces() called first or had nested async calls I'd understand what you're asking for better.

Collaborator

benjamingr commented Dec 16, 2014

Yeah, I understand what stack trace I'm seeing, what I don't understand is why you find it useful in this case - now if you had Promise.longStackTraces() called first or had nested async calls I'd understand what you're asking for better.

@nomcopter

This comment has been minimized.

Show comment
Hide comment
@nomcopter

nomcopter Dec 16, 2014

In the first case, it tells me the exact line that the rejected promise was produced. In the second case, all it gives me is the error message and a line number deep within bluebird. Here is an example of how the stack trace is useful:

screen shot 2014-12-16 at 2 26 37 pm

In a large project, if things break and all I see is an error with a line deep in bluebird, I wouldn't know where to even start looking.

nomcopter commented Dec 16, 2014

In the first case, it tells me the exact line that the rejected promise was produced. In the second case, all it gives me is the error message and a line number deep within bluebird. Here is an example of how the stack trace is useful:

screen shot 2014-12-16 at 2 26 37 pm

In a large project, if things break and all I see is an error with a line deep in bluebird, I wouldn't know where to even start looking.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 17, 2014

Owner

Since the .done() error is just thrown in the process normally (throw e)it doesn't know how to format it, the possibly unhandled error formatting is actually very custom work and the red color is from using .console.error

Owner

petkaantonov commented Dec 17, 2014

Since the .done() error is just thrown in the process normally (throw e)it doesn't know how to format it, the possibly unhandled error formatting is actually very custom work and the red color is from using .console.error

@nomcopter

This comment has been minimized.

Show comment
Hide comment
@nomcopter

nomcopter Dec 17, 2014

That custom work is very helpful and essential for debugging. Without it my workflow when this happens is:

  1. Breakpoint on bluebird.js:174
  2. Retrigger error
  3. Check res.e.stack

Maybe add a configurable option (or default) to log the stack trace before it throws the error from a .done()ed promise?

nomcopter commented Dec 17, 2014

That custom work is very helpful and essential for debugging. Without it my workflow when this happens is:

  1. Breakpoint on bluebird.js:174
  2. Retrigger error
  3. Check res.e.stack

Maybe add a configurable option (or default) to log the stack trace before it throws the error from a .done()ed promise?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 17, 2014

Owner

I'd prefer just automatically doing it by default without even exposing configuration but I never personally use .done() so I am not sure if that's ok to .done() users

Owner

petkaantonov commented Dec 17, 2014

I'd prefer just automatically doing it by default without even exposing configuration but I never personally use .done() so I am not sure if that's ok to .done() users

@nomcopter

This comment has been minimized.

Show comment
Hide comment
@nomcopter

nomcopter Dec 17, 2014

I agree that it should be automatic. It is important that it doesn't look like two errors were thrown though.

We use .done() in our project in to be explicit when we have finished working with a particular promise - especially because of coffeescript's implicit returns. It also matches the Promises/A+ spec a bit more since more other Promise libraries only actually throw errors on rejected Promises if .done() has been called.

nomcopter commented Dec 17, 2014

I agree that it should be automatic. It is important that it doesn't look like two errors were thrown though.

We use .done() in our project in to be explicit when we have finished working with a particular promise - especially because of coffeescript's implicit returns. It also matches the Promises/A+ spec a bit more since more other Promise libraries only actually throw errors on rejected Promises if .done() has been called.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 17, 2014

Owner

Do you have any ideas how to make it obvious they are not two errors? Remember that the log cannot come after the error so we can't do e.g. Additional stack trace:

Owner

petkaantonov commented Dec 17, 2014

Do you have any ideas how to make it obvious they are not two errors? Remember that the log cannot come after the error so we can't do e.g. Additional stack trace:

@nomcopter

This comment has been minimized.

Show comment
Hide comment
@nomcopter

nomcopter Dec 17, 2014

Ideas (best first):

  1. Simply console.warn saying something like ".done() called on rejected promise, throwing to window" and the stacktrace before throwing it.
  2. Append the additional stack trace to the actual error (res.e.message += trace), but people might be expecting any thrown errors' messages to remain intact.
  3. Attach the stack trace to a global and console.warn its location before throwing the error. i.e. Promise.getStackTraceForError(errorId) or window.lastBluebirdStack

nomcopter commented Dec 17, 2014

Ideas (best first):

  1. Simply console.warn saying something like ".done() called on rejected promise, throwing to window" and the stacktrace before throwing it.
  2. Append the additional stack trace to the actual error (res.e.message += trace), but people might be expecting any thrown errors' messages to remain intact.
  3. Attach the stack trace to a global and console.warn its location before throwing the error. i.e. Promise.getStackTraceForError(errorId) or window.lastBluebirdStack
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 18, 2014

Owner

Turns out I can log right away after the error is thrown, so it looks like this atm:

done

Owner

petkaantonov commented Dec 18, 2014

Turns out I can log right away after the error is thrown, so it looks like this atm:

done

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 18, 2014

Owner

Actually I will change it to console.warn, the expanding arrow button is confusing

done

Owner

petkaantonov commented Dec 18, 2014

Actually I will change it to console.warn, the expanding arrow button is confusing

done

@nomcopter

This comment has been minimized.

Show comment
Hide comment
@nomcopter

nomcopter Dec 18, 2014

Awesome. Thanks a lot. Why did you opt to only output this if longStackTraces have been enabled?

nomcopter commented Dec 18, 2014

Awesome. Thanks a lot. Why did you opt to only output this if longStackTraces have been enabled?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 18, 2014

Owner

Because without it there wouldn't be any additional stack trace

Owner

petkaantonov commented Dec 18, 2014

Because without it there wouldn't be any additional stack trace

@nomcopter

This comment has been minimized.

Show comment
Hide comment
@nomcopter

nomcopter Dec 18, 2014

Huh, I get the same in Chrome regardless. Not sure why.
screen shot 2014-12-18 at 7 14 27 pm

(I get the same output even if I call Promise.longStackTraces() before creating the first promise)

nomcopter commented Dec 18, 2014

Huh, I get the same in Chrome regardless. Not sure why.
screen shot 2014-12-18 at 7 14 27 pm

(I get the same output even if I call Promise.longStackTraces() before creating the first promise)

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 18, 2014

Owner

I changed the condition to if ("stack" in e) {

Owner

petkaantonov commented Dec 18, 2014

I changed the condition to if ("stack" in e) {

@nomcopter

This comment has been minimized.

Show comment
Hide comment
@nomcopter

nomcopter Dec 18, 2014

Perfect. Thank you so much for the quick fix as always! :)

nomcopter commented Dec 18, 2014

Perfect. Thank you so much for the quick fix as always! :)

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