Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Expanded BigInt support #2
Expanded BigInt support #2
Changes from 2 commits
992882b
eeeec65
6039e48
4f73b37
c846d0d
d346cdc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
useBigInt64
option was being conflated for 2 things - one was encoding bigint's as int64, the other was encoding bignumber
's asint64
rather thanfloat64
so I've added configuration to support both (useInt64
andforceBigIntToInt64
). The default config (bothfalse
) will have the same behaviour as before.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, on reflection, maybe the default behaviour here is different and I need to make
useInt64
true by default?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my opinion. The upstream library's decision to encode numbers above the uint32 max or below the int32 min if
useBigInt64
is false is nonsensical. Javascript numbers can safely represent integers outside of this range without using bigints, and the decision to encode these as floats when using the default encoding params (thereby confusing every other non-JS msgp decoder) is mind boggling.I'd get rid of this condition entirely.
At this point our top priority is delivering a library with enhanced features that are easy to use and make sense. We're only in this position because the upstream library has failed us. Let's focus on correcting the behavior over trying to be compatible with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to drop compatibility with the core library if you are. My main concern is that it means we do have to maintain this library long term since it makes it less likely that the core library would accept our changes if they take on maintenance of that library again.
Side-note: should we raise all of these PRs to the core library as well as this one do you think or not worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the fact there is a forceIntegerToFloat means that it's still possible to force a BigInt to a float.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should fix this inconsistency, since the docstring for
forceBigIntToInt64
saysforceIntegerToFloat
doesn't affect bigints. I'm fine with qualifying that dosctring to say that it doesn't affect bigints if that option is enabled.For the most part each of these PRs already exists upstream, right? I guess the main difference is that they're not based on the most recent changes to the main branch.
My current intention is to release this fork under a different name, then post a link to it on each of the upstream PRs. Hopefully then we'll at least help others in similar situations.
Ideally yes, we could merge all of these features back upstream, but I'm not counting on that as a viable option in the short term.