Why is this library overwriting Error type? about nodejs-depd HOT 5 CLOSED

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.

from nodejs-depd.

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

from nodejs-depd.

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.

from nodejs-depd.

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.

from nodejs-depd.

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

from nodejs-depd.