Why is this library overwriting Error type?

[purpledrgn]

Is there no way to avoid this function?

function getStack () {
  var limit = Error.stackTraceLimit
  var obj = {}
  var prep = Error.prepareStackTrace

  Error.prepareStackTrace = prepareObjectStackTrace
  Error.stackTraceLimit = Math.max(10, limit)

  // capture the stack
  Error.captureStackTrace(obj)

  // slice this function off the top
  var stack = obj.stack.slice(1)

  Error.prepareStackTrace = prep
  Error.stackTraceLimit = limit

  return stack
}

More specifically the native Error type overwrites.

I'm not against overwrites to native types but this sort of unrequested overwrite is just annoying. Overwrites are either at the user level or by request if libraries offer them (usually the case for unit test libraries).

What happens with cases such as this particular case is that it (no surprise) bumps heads with other requested overwrites resulting in hard to track errors such as callSite.getFileName is not a function. Since this included by things such as body-parser, you are forcing this overwrite on everyone who uses it.

In addition the apis used there, seem to be V8 specific, throwing even more spanners around.

[dougwilson]

Hi @purpledrgn I'm sorry this is causing issues for you. It's not trying to actually change the global Error object, but it's just trying to use the Node.js API to capture stack traces

• https://nodejs.org/dist/latest-v11.x/docs/api/errors.html#errors_error_capturestacktrace_targetobject_constructoropt
• https://nodejs.org/dist/latest-v11.x/docs/api/errors.html#errors_error_stacktracelimit

etc.

If there is a better way to accomplish getting the stack, I'm all for it. There is a pretty extensive test suite around this module which should help highlight why this is there; it was added to fix exceptions that were being encountered by users. If you're also encountering exceptions from this, I'd love to get it fixed!

If you can provide a reproduction case I can look into, I can surely help to find a workable solution for you if you like, otherwise you can always make a pull request as well.

[purpledrgn]

The safest way to fix it is to actually check if the type of Error is actually what you think it is. If it isn't ignore it and short circuit out with some default. Checking function/property presence and output sanity of said functions is probably the best way to confirm; and you just might get what you need.

The problem with Error.captureStackTrace is that to actually pass the functionality to something that overwrites Error it is not a trivial problem. Easy to get wrong. Easy to forget to do.

The following is probably the only way to actually ensure your code works:

const NativeError = Error;

class Overwrite {
    constructor() {
        this.stack = "\n\n\n"
    }
}

Object.defineProperty(Overwrite, 'prepareStackTrace', {
    set: handle => {
        NativeError.prepareStackTrace = handle;
    },
    get: () => {
        return NativeError.prepareStackTrace;
    }
});

Overwrite.captureStackTrace = NativeError.captureStackTrace;

Object.defineProperty(Overwrite, 'stackTraceLimit', {
    set: value => {
        NativeError.stackTraceLimit = value;
    },
    get: () => {
        return NativeError.stackTraceLimit;
    }
});

////////////////////////////////////////////////////////////////////////////////////

Error = Overwrite;

////////////////////////////////////////////////////////////////////////////////////

function prepareObjectStackTrace(obj, stack) {
    return stack;
}

function getStack () {
    var limit = Error.stackTraceLimit
    var obj = {}
    var prep = Error.prepareStackTrace

    Error.prepareStackTrace = prepareObjectStackTrace
    Error.stackTraceLimit = Math.max(10, limit)

    // capture the stack
    Error.captureStackTrace(obj)

    // slice this function off the top
    var stack = obj.stack.slice(1)

    Error.prepareStackTrace = prep
    Error.stackTraceLimit = limit

    return stack
}

function callSiteLocation (callSite) {
    var file = callSite.getFileName() || '<anonymous>'
    var line = callSite.getLineNumber()
    var colm = callSite.getColumnNumber()

    if (callSite.isEval()) {
        file = callSite.getEvalOrigin() + ', ' + file
    }

    var site = [file, line, colm]

    site.callSite = callSite
    site.name = callSite.getFunctionName()

    return site
}

function a() {
    var stack = getStack()
    var site = callSiteLocation(stack[1])
    return site;
}

function b() {
    return a();
}

console.log(b());

Any implementation that doesn't do the setter/getter hack there will just fail in mystifying ways. How many times have you written Object.defineProperty like that? I'd wager a lot of random implementations will just not do that. Your code there is also heavily leaning on it, so if it doesn't work it will fail.

You linked to nodejs docs, but only 2 of the 3 are listed there. I don't see any mention of Error.prepareStackTrace in there. It's documented in V8, but not in the official nodejs docs. I have no idea if that's intentional or an omission. Either way, given the example above I recommend refactoring it out because implementations will very likely link back to the captureStackTrace function on the native Error object, but they're unlikely to implement the finesse required for this undocumented feature.

• https://v8.dev/docs/stack-trace-api#customizing-stack-traces

[dougwilson]

Yea, I can definitely make changes! If you can provide a reproduction case I can look into, I can surely help to find a workable solution for you if you like, otherwise you can always make a pull request as well along with tests added to the test suite.

[purpledrgn]

Simply comment out in the example I gave you the following:

Object.defineProperty(Overwrite, 'prepareStackTrace', {
    set: handle => {
        NativeError.prepareStackTrace = handle;
    },
    get: () => {
        return NativeError.prepareStackTrace;
    }
});

You'll get a perfect reproduction case.

Besides a, b and the Overwrite bits everything is copy-pasted from your code.

Like I said above I think it's fair to expect overwrites to Error to provide captureStackTrace since it's documented in nodejs docs, but prepareStackTrace is just not documented and too rarely used. Your code is the first time I've ever seen it used. You can also comment out the stackTraceLimit one but that doesn't make much of a difference either way.

[dougwilson]

Gotcha. I'm not going to support such an environment who is overwriting the global Error if that is what is happening.