Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add support for bigint #970

Closed
wants to merge 7 commits into from
Closed

add support for bigint #970

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a8c5e7e
add support for bigint
alexey-sh Feb 16, 2021
d01cb27
fix bigint support
alexey-sh Feb 17, 2021
c594f4c
add 2 simple tests for bigint
alexey-sh Feb 17, 2021
5584d58
add support bigint for nested objects
alexey-sh Feb 17, 2021
3632ee3
add test for bigint in nested objects
alexey-sh Feb 17, 2021
0822130
Update test/basic.test.js
alexey-sh Feb 17, 2021
eb64c0d
Update test/basic.test.js
alexey-sh Feb 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/tools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ function asJson (obj, msg, num, time) {
case 'undefined':
case 'function':
continue
case 'bigint':
value = (stringifier || asString)(value.toString() + 'n')
break
case 'number':
/* eslint no-fallthrough: "off" */
if (Number.isFinite(value) === false) {
Expand Down Expand Up @@ -396,7 +399,12 @@ function final (logger, handler) {

function stringify (obj) {
try {
return JSON.stringify(obj)
return JSON.stringify(obj, (key, value) => {
if (typeof value === 'bigint') {
return value.toString() + 'n'
}
return value
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to slow us down significanfly. I'm -1 to us landing this feature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deep object benchmarks confirm this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} catch (_) {
return stringifySafe(obj)
}
Expand Down
45 changes: 45 additions & 0 deletions test/basic.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,51 @@ test('handles objects with null prototype', async ({ same }) => {
})
})

test('normalize bigint to string', async ({ same }) => {
const stream = sink()
const instance = pino(stream)
const bigInt = 1n
instance.info({ bigInt })
const result = await once(stream, 'data')
delete result.time
same(result, {
pid,
hostname,
level: 30,
bigInt: `${bigInt.toString()}n`
})
})

test('normalize nested object with bigint to string', async ({ same }) => {
const stream = sink()
const instance = pino(stream)
const bigInt = { a: { b: 1n } }
instance.info({ bigInt })
const result = await once(stream, 'data')
delete result.time
same(result, {
pid,
hostname,
level: 30,
bigInt: { a: { b: '1n' } }
})
})

test('object and format bigInt property', async ({ same }) => {
const stream = sink()
const instance = pino(stream)
instance.info({ answer: 42n }, 'foo %s', 1n)
alexey-sh marked this conversation as resolved.
Show resolved Hide resolved
const result = await once(stream, 'data')
delete result.time
same(result, {
pid,
hostname,
level: 30,
msg: 'foo 1',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this have the n suffix?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh idk. is it important? would you like me to add the suffix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

answer: '42n'
alexey-sh marked this conversation as resolved.
Show resolved Hide resolved
})
})

test('pino.destination', async ({ same }) => {
const tmp = join(
os.tmpdir(),
Expand Down