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

Support toJSON as a fallback on non-object types (BigInt) #28

Closed

Conversation

papandreou
Copy link

@papandreou papandreou commented May 15, 2022

Hi!

My application uses BigInt heavily, which results in Could not encode with this library. I tried to follow this piece of advice and specified a BigInt.prototype.toJSON. Unfortunately that doesn't work, because the typeof operator returns bigint, and this library only supports toJSON for values where typeof returns object.

I saw #25 but thought it'd be worthwhile to consider a more minimalistic approach.

Drive-by: Don't test with node.js 18, as that seems to break Travis. (Should probably switch to a different CI provider)

darrachequesne pushed a commit that referenced this pull request May 20, 2022
Another possible implementation was to always encode BigInt values as
strings, but that does not allow to encode it as numbers to save a few
bytes.

This solution is also more future-proof, in case some other types are
added to JavaScript.

Related:

- #28
- #25
- #24
@darrachequesne
Copy link
Owner

That sounds reasonable, thanks 👍

Merged as 9ec1fcd.

@papandreou
Copy link
Author

@darrachequesne, thanks a lot! Mind doing a new release? 🤗

@darrachequesne
Copy link
Owner

@darrachequesne darrachequesne added this to the 3.0.0 milestone May 22, 2022
@papandreou
Copy link
Author

@darrachequesne, thanks a lot! I'm using this library via @socket.io/redis-emitter, which depends on notepack.io@~2.1.0: https://github.com/socketio/socket.io-redis-emitter/blob/d62af2c9ec45cf991cb32c286c511354f11ea0b1/package.json#L23

I see your name on that library as well, so just putting in a good word for a new release that bumps that dependency as well 🙏 😅

@papandreou
Copy link
Author

@darrachequesne, thanks a lot! I'm using this library via @socket.io/redis-emitter, which depends on notepack.io@~2.1.0: https://github.com/socketio/socket.io-redis-emitter/blob/d62af2c9ec45cf991cb32c286c511354f11ea0b1/package.json#L23

I see your name on that library as well, so just putting in a good word for a new release that bumps that dependency as well 🙏 😅

Opened a PR here for that update: socketio/socket.io-redis-emitter#113

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

Successfully merging this pull request may close these issues.

2 participants