Preserve original stack trace(s)

[aseemk]

Hi there,

Great work on this lib! I really like the intent and most things about it.

One thing I find lacking, that we rely on a lot in our production Node apps at @FiftyThree, is the original stack trace of the very lowest-level error. Since that's typically the root cause, we need to know where it's actually happening.

I noticed in issue #21 that someone else wondered about this too. More than just documenting it, though, might you consider exposing VError's/WError's stack property dynamically, to include cause stacks?

E.g. just as a rough example, untested:

function VError(...) {
    // ...
    Error.captureStackTrace(this, ctor);
    this._stack = this.stack;
    // ...
}

Object.defineProperty(VError.prototype, 'stack', {
    get: function () {
        var str = this._stack;
        if (this.jse_cause) {
            str += '\nCaused by:\n' + this.jse_cause.stack;
        }
        return str;
    }
}

This is similar to e.g. Java stack traces.

What do you think of something like this? Thanks!

[davepacheco]

One problem with that approach is that it fetches the formatted stack trace in the constructor. Although I haven't gathered performance data myself, many people consider this too expensive to be suitable for production code, because that's the expensive part of gathering stack traces (and I believe that's why "stack" is normally defined using a getter).

It also sort of changes the semantics of "stack", which normally contains the stacktrace where the Error was instantiated. I'm not sure how much of a problem that is.

[aseemk]

Yep, that was just a rough example. Easy to optimize the stack lookup away. E.g.:

function VError(...) {
    // ...
    Error.captureStackTrace(this, ctor);
    // (notice no more reading `stack` here)
    // ...
}

var stackGetter = Object.getOwnPropertyDescriptor(Error.prototype, 'stack').get;

Object.defineProperty(VError.prototype, 'stack', {
    get: function () {
        var str = stackGetter.call(this);
        if (this.jse_cause) {
            str += '\nCaused by: ' + this.jse_cause.stack;
        }
        return str;
    }
}

—

Re: semantics of stack: sorry if I was unclear — this would only be adding info to the native stack, not removing or changing any of the original info.

E.g. before:

VError: foo: VError: lorem: ENOENT, no such file or directory './sadsd'
    at bar (baz.js:1:1)
    ...

Vs. after:

VError: foo: VError: lorem: ENOENT, no such file or directory './sadsd'
    at bar (baz.js:1:1)
    ...
Caused by: VError: lorem: ENOENT, no such file or directory './sadsd'
    at ipsum (dolor.js:2:2)
    ...
Caused by: ENOENT, no such file or directory './sadsd'
    at Object.fs.statSync (fs.js:695:18)
    ...

(Again, rough example. E.g. might make sense to repeat the cause error name and/or message on those "Caused by" lines. Turns out that's already naturally the case.)

[veselov]

So much +1

[cappslock]

Another approach, why not add a method to VError's prototype which does this? You could then do error.stack or error.getFullStack(), for example.

[davepacheco]

I like having something to denote the full cause chain, but I'm still squeamish to change the format of .stack from what people typically expect. I'd be all for having a .getStackWithCauses() or the like.

[cappslock]

I'll try to get a PR open within the next couple of days with this and some tests.

[pwmckenna]

any progress on this? I'd be happy to hop in to help

[cappslock]

I didn't get a chance to start, though it's in my backlog. Go for it if you want!

[pwmckenna]

https://github.com/davepacheco/node-verror/pull/24

couldn't quite figure out the style utilities, so I wouldn't be surprised if I ran afoul of the standards. let me know and I can fix it up.

[davepacheco]

Landed via #38.