Add bun compatibility

[AaronDewes]

Fixes #1802. I'm not sure if this is something you want, but it's an easy fix that doesn't break anything. This has been tested in both Node and Bun.

The underlying issue in Bun is probably harder for their team to fix, because it would very likely require changes to WebKit.

[kibertoad]

Bun doesn't support Errors? Or Errors don't have stack?

[AaronDewes]

Bun doesn't support Errors? Or Errors don't have stack?

Currently, Error.stack ignores Error.prepareStackTrace because of a bug in Bun.

[AaronDewes]

@robobun

function noOpPrepareStackTrace (_, stack) {
  return stack
}

function getCallers() {
  const originalPrepare = Error.prepareStackTrace
  Error.prepareStackTrace = noOpPrepareStackTrace
  const stack_default = new Error().stack
  const tmpError = {};
  Error.captureStackTrace(tmpError)
  const { stack } = tmpError;
  Error.prepareStackTrace = originalPrepare;
  console.log(`new Error().stack: typeof stack.default == ${typeof stack_default}`);
  console.log(Error.captureStackTrace(targetObj); typeof targetObj.stack == ${typeof stack}`);
}

function main() {
  getCallers();
}

main();

[AaronDewes]

I have no idea if this bot still works, but the above code is an example for the bug in bun.

[kibertoad]

If it's just a bug in Bun, I'm -1 on this change, this seems hacky and brings no benefits to the library itself.

update: thought about it more, I'm OK with this, if there is Bun ticket linked in todo in comment next to this change with explanation.

[kibertoad]

@AaronDewes please see updated comment. People are starting to experiment with bun, we probably don't want to block them.

can you benchmark perf of both approaches?

[AaronDewes]

It looks like it decreases performance:

function noOpPrepareStackTrace (_, stack) {
    return stack
}

const originalPrepare = Error.prepareStackTrace
Error.prepareStackTrace = noOpPrepareStackTrace

// Method 1: Using new Error().stack
console.time('Method 1');
for (let i = 0; i < 100000; i++) {
  const stack = new Error().stack;
}
console.timeEnd('Method 1');

// Method 2: Using Error.captureStackTrace()
console.time('Method 2');
for (let i = 0; i < 100000; i++) {
  let tmpError = {};
  Error.captureStackTrace(tmpError);
  const { stack } = tmpError;
}
console.timeEnd('Method 2');

Error.prepareStackTrace = originalPrepare

Node:

Method 1: 231.382ms
Method 2: 249.456ms

Bun:

[73.06ms] Method 1
[100.05ms] Method 2

[mcollina]

I'm -1 for this change. Open a bug in Bun. This also broke all the tests, so possibly there is something wrong too in your implementation.

As long as Bun does not support running tap tests, it's very hard for us to land these PR... because there is no way to know that pino is actually working.

[mcollina]

https://x.com/jarredsumner/status/1700497869160268155?s=46&t=22eoAstJVk5l46KQXYEk5Q

[AaronDewes]

Thanks! I was aware that Bugs in Bun should be fixed, but I thought this is a simple fix for Pino too.

I wouldn't file an issue for bugs in Bun to JS projects, but if something can be fixed easily, I thought a PR would be helpful, but I understand your reasoning.

[kibertoad]

@AaronDewes Appreciate you sending in the PR either way! It's always great to see people doing something about topics that they care about.

[christophemarois]

As an update for interested future readers, this will apparently be fixed in bun 1.0.3 oven-sh/bun#4280 (comment)

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.