add support for bigint #970
Conversation
|
I'm not sure we should land this as JSON.stringify() does not support this correctly either. |
I'm not sure there is a way to make it faster than JSON.stringify. |
I added some tests. Please have a look |
| pid, | ||
| hostname, | ||
| level: 30, | ||
| msg: 'foo 1', |
There was a problem hiding this comment.
Why doesn't this have the n suffix?
There was a problem hiding this comment.
tbh idk. is it important? would you like me to add the suffix?
There was a problem hiding this comment.
The other outputs in this PR have the suffix. I'm asking about it from a consistency standpoint. How will an interpreter know that it is a BigInt without the suffix?
Current master branch benchPR branch bench |
| return value.toString() + 'n' | ||
| } | ||
| return value | ||
| }) |
There was a problem hiding this comment.
This is going to slow us down significanfly. I'm -1 to us landing this feature.
There was a problem hiding this comment.
The deep object benchmarks confirm this.
There was a problem hiding this comment.
This is going to slow us down significanfly.
have you checked the case then JSON.strigify throws an error? I bet it goes down a lot.
I'm -1 to us landing this feature.
and -{a lot of users} because luck of bigint support.
––––
I guess it's possible to cofigure pino to skip bigint check. does it sound fine for you guys?
There was a problem hiding this comment.
Thanks for the pull request. We have had a discussion and have determined that BigInt needs better support in general. This implementation negatively impacts our benchmarks even when a BigInt is not present.
PR suggestion Co-authored-by: James Sumners <james@sumners.email>
PR suggestion Co-authored-by: James Sumners <james@sumners.email>
No description provided.