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.

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

The output is not JSON.stringify-safe #55

Open
tjconcept opened this issue Jan 24, 2022 · 6 comments
Open

The output is not JSON.stringify-safe #55

tjconcept opened this issue Jan 24, 2022 · 6 comments

Comments

@tjconcept
Copy link

tjconcept commented Jan 24, 2022

import {serializeError} from 'serialize-error'

const err = new Error
err.timestamp = process.hrtime.bigint()

JSON.stringify(serializeError(err))

// Uncaught TypeError: Do not know how to serialize a BigInt
//     at JSON.stringify (<anonymous>)

The fix seems simple if doing something akin to buffers (though the value could be embedded). Am I missing something? If fixed by a PR, could it be backported no a non-ESM version?

@sindresorhus
Copy link
Owner

This package does not guarantee JSON.stringify compatibility, but it's a nice to have.

I think we could convert BigInt to a string. That's the only way to preserve its value.

@sindresorhus
Copy link
Owner

could it be backported no a non-ESM version?

No, I only backport critical security issues.

@tjconcept
Copy link
Author

This package does not guarantee JSON.stringify compatibility, but it's a nice to have.

That's currently my only use case. Are there many others that don't also require JSON compatibility?

How do you feel about making it a guarantee? I imagine doing so by switching through (at least) all JSON types (plus special cases like Buffer, BigInt, etc.) and then defaulting to String().

I think we could convert BigInt to a string. That's the only way to preserve its value.

Makes sense. How about [BigInt 123456789]?

No, I only backport critical security issues.

Would you publish a version if someone else backported it? 😅

@sindresorhus
Copy link
Owner

That's currently my only use case. Are there many others that don't also require JSON compatibility?

Other serialization formats. I didn't want it to be too opinionated. The goal was to simplify an error to a plain object with primitive values. But I do agree JSON is probably the main targets, so I guess making it JSON compatible by default makes sense.

Makes sense. How about [BigInt 123456789]?

I think it depends on your use-case. Whether you need to just display the error or you want it transferred and parsed somewhere else. Your suggestion looks a little bit too close to object type output: [object BigInt]. Maybe just 123456789n as a string.

Would you publish a version if someone else backported it?

I get asked about this all the time. I'm not really interested in spending all my time backporting or handle backports. It's a slippery slope. If I say yes to this, I'm obliged to handle backporting of any bugs or missing use-cases with this feature. Nothing is stopping you from backporting in your own fork and using that though. You can use Git forks directly from npm.

@tjconcept
Copy link
Author

Maybe just 123456789n as a string.

I think that's just as fine. Since we can't do perfect deserialization anyway, it's simply about doing something more useful than stripping the value to me.

too close to object

That was actually on purpose because buffers are serialized with that style. It's not important to me though - both versions can be parsed and any version will require a positional expectation for BigInts to parse anyway.

I'm not really interested in spending all my time backporting or handle backports.

I totally get it. I'd do the backporting. I'm asking if you would:

  • Create a branch for v8
  • Review and merge a backporting PR
  • Publish in npm

If we successfully land a PR here to add perfect JSON compatibility. Not because a fork wouldn't perfectly solve my problem, but because I want as many people as possible to benefit from it.

@fregante
Copy link
Contributor

fregante commented Feb 13, 2022

While it seems to make sense at first, I don’t think this should be part of serialize-error.

Calling JSON.stringify(serializeError(error)) is not different from JSON.stringify(bigIntArray): in both cases it's your responsibility to guarantee that the next step is compatible with the input using the third parameter of stringify.

I think serializing other types is a slippery slope before serialize-error turns into serialize-javascript

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants