check before running captureStackTrace

[salvocanna]

This little lib is used by both node and frontend, and because some browser might not implement captureStackTrace, it will throw an exception, badly.

This PR is meant to safe check / fix this call and make it safe for the web 🕷🕸👌

[coveralls]

Coverage increased (+1.2%) to 58.824% when pulling fd1a81e on check-capture-stack-trace into 85eeb58 on master.

[deepakrb]

We should move to using the raw Error.stack instead of captureStackTrace (which is more V8 specific and not widely supported)

Suggested change

	if (Error.captureStackTrace)
	Error.captureStackTrace(this, this.constructor);
	if (Error().stack)
	this.stack = Error().stack;

captureStackTrace doesn't seem to do more than setting the .stack variable ourselves. It does support hiding some calls from the stack trace (anything from this.constructor in our case). Moving to the raw .stack call seems like a decent solution to get a wider supported stack trace.

https://nodejs.org/api/errors.html#errors_error_capturestacktrace_targetobject_constructoropt

Thoughts?

[deepakrb]

The purpose of this change is to fix error resolution in the frontend.

We're aware that because captureStackTrace is not widely supported Firefox (and other browsers) would fail to resolve backend errors and their messages correctly.

Although this is a -node package it also seems to be used in the frontend so its probably best we add support for other web browsers which encounter this error.

[salvocanna]

I was having a look at the docs and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/stack seems to be non-standard too, which makes me wonder about just using a polyfill like https://www.npmjs.com/package/error-polyfill
What do you think?

[g-wilson]

It says non-standard but seems to have extremely good support?

Why don't we continue with captureStackTrace if it exists, then check Error.stack

[0xdeafcafe]

Yeah, the Error.stack support is rather wonky. We also don't really need the stack for the kind of errors we throw with this package on the web.

[deepakrb]

Yeah, the Error.stack support is rather wonky. We also don't really need the stack for the kind of errors we throw with this package on the web.

I would rather we just used the .stack method (which is generally supported) behind a check than use a V8 specific API behind a check. The V8 captureStackTrace is so simple we may as well use the more supported Error().stack

[salvocanna]

Stack seems to be already there by default it's not serialisable, adding it manually with .stack = would mean it becomes serialisable (which I guess we could fix) but I fear this might introduce some other unexpected changes for backend and I fear opening that can of worms.
I'd merge it like this as it is