Expanded BigInt support #2
Conversation
Modified them to properly support bigint and convert them to TypeScript Adding VS Code support for mocha
src/Encoder.ts
Outdated
| @@ -200,7 +213,7 @@ export class Encoder<ContextType = undefined> { | |||
| // uint 32 | |||
| this.writeU8(0xce); | |||
| this.writeU32(object); | |||
| } else if (!this.useBigInt64) { | |||
| } else if (this.useInt64) { | |||
There was a problem hiding this comment.
The useBigInt64 option was being conflated for 2 things - one was encoding bigint's as int64, the other was encoding big number's as int64 rather than float64 so I've added configuration to support both (useInt64 and forceBigIntToInt64). The default config (both false) will have the same behaviour as before.
There was a problem hiding this comment.
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.
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.
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.
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.
Also, the fact there is a forceIntegerToFloat means that it's still possible to force a BigInt to a float.
We should fix this inconsistency, since the docstring for forceBigIntToInt64 says forceIntegerToFloat doesn't affect bigints. I'm fine with qualifying that dosctring to say that it doesn't affect bigints if that option is enabled.
should we raise all of these PRs to the core library as well as this one do you think or not worth it?
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.
|
Closing and reopening to trigger github actions |
src/Encoder.ts
Outdated
| @@ -200,7 +213,7 @@ export class Encoder<ContextType = undefined> { | |||
| // uint 32 | |||
| this.writeU8(0xce); | |||
| this.writeU32(object); | |||
| } else if (!this.useBigInt64) { | |||
| } else if (this.useInt64) { | |||
There was a problem hiding this comment.
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.
BREAKING CHANGE: `useBigInt64` deprecated from Encoder and > 32 bit ints now get encoded as a uint64 rather than float64
|
FYI I pushed another commit with the tweaks based on feedback |
src/Encoder.ts
Outdated
| @@ -200,7 +213,7 @@ export class Encoder<ContextType = undefined> { | |||
| // uint 32 | |||
| this.writeU8(0xce); | |||
| this.writeU32(object); | |||
| } else if (!this.useBigInt64) { | |||
| } else if (this.useInt64) { | |||
There was a problem hiding this comment.
Also, the fact there is a forceIntegerToFloat means that it's still possible to force a BigInt to a float.
We should fix this inconsistency, since the docstring for forceBigIntToInt64 says forceIntegerToFloat doesn't affect bigints. I'm fine with qualifying that dosctring to say that it doesn't affect bigints if that option is enabled.
should we raise all of these PRs to the core library as well as this one do you think or not worth it?
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.
Additional int testing
|
@jasonpaulos Ready for final review |
Took msgpack#211, rebased against latest
main.BigInt support had been added (in a very basic way) in msgpack library, these changes expand the int handling to have more configurability and sophistication, while maintaining backwards compatibility with latest msgpack.
I also refactored the tests to pull in a whole heap of external dependencies that need to be here so they are more easily understood and modified (I had to modify them to handle BigInt to avoid tests breaking).