-
Notifications
You must be signed in to change notification settings - Fork 29
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
Large nums as strings #225
Conversation
Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matthew Whitehead <[email protected]>
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.
Great stuff - I think just documenting that long number might appear as strings in the docs is fine for now
export function parseLargeNumbersAsStrings(value: any) { | ||
return isSafeNumber(value, { approx: false }) | ||
? parseFloat(value) // Smaller numbers are kept as Javascript numbers | ||
: new LosslessNumber(value).toString(); // Large numbers are safely stringified | ||
} | ||
|
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 based on this example https://github.com/josdejong/lossless-json?tab=readme-ov-file#lossless-json couldn't we just call parse()
and that's 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.
Or is it because lower down in the react component we are actually doing some manipulation?
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 problem with just calling parse()
is that small numbers all end up as LosslessNumber
s as well and are therefore stringified in the UI. Which I don't think we want. So the extra number parser allows us to retain Javascript numbers
for smaller numbers.
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.
But they don't look like stringified here
// LosslessJSON.parse will preserve all numbers and even the formatting:
console.log(stringify(parse(text)))
// '{"decimal":2.370,"long":9123372036854000123,"big":2.3e+500}'
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.
So it's because we want the large numbers as a string (instead of a BigInt which would render in the UI as 1000000000000001n
). In their example they are using their own stringify
to render it as a large number without a string, but we would need to refactor a lot of the codebase to use that instead of our existing JSON serialisation logic.
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.
Got caught out that stringify is actually their own function
Relates to hyperledger/firefly#1568
With large JSON numbers supported in FF (see hyperledger/firefly#1569 and hyperledger/firefly#1581) the FireFly UI received large numbers correctly but then lost precision when rendering them.
For example the following
Data
viewer:which was rendered from the following JSON payload from FF:
This PR uses the MIT-licenses package
lossless-json
to replace theJSON.parse
step in allfetch(...)
calls. It stringifies numbers that are too large to be represented as a Javascriptnumber
e.g.which is from the same JSON payload as above.
The parser leaves all other data untouched (i.e. it returns the same JSON as
JSON.parse
except for the case where a number is determined to be a number that cannot be safely represented as a Javascriptnumber
).Here is an example of a small number, which is represented exactly as it would be before this PR:
Here is an example of a string, also represented exactly as it would be before this PR:
I think representing numbers in this way as strings is definitely preferable to showing incorrect data, but it might be useful to FF users to show some indication in UI panels that data has been manipulated to render it correctly. I'm considering that work out of scope for this fix but mentioning here for discussion.