Uh oh!
There was an error while loading. Please reload this page.
patch(bigint): handle bigint in responses - #5445
Conversation
converts bigints to numbers recursively in case of objects/arrays Signed-off-by: Bhavya Dhiman <bhavyadhiman7@gmail.com>
dougwilson
commented
Feb 5, 2024
Doesn't this... break big numbers silently? Can you add a test for an actual big number? Or any tests at all? This seems highly dangerous unless perhaps it will error out for big number that cannot be represented as a Also curious on how this may break existing uses of the replacer for folks who many be doing things like bigints already (lile coverting to a string), so also seems to be a major breaking change too. |
| case 'boolean': | ||
| case 'number': | ||
| case 'object': | ||
| case 'bigint': |
There was a problem hiding this comment.
It seems strange to add a case here for the type and then still check the type again in the case body. Should this not simply be it's own case and body with a break?
There was a problem hiding this comment.
adding bigint seperatlely would have made more sense! got it!
| // If it's an array, loop over its elements and convert them recursively | ||
| if (Array.isArray(obj)) { | ||
| for (var index = 0; index < obj.length; index += 1) { | ||
| obj[index] = convertBigIntsToNumbers(obj[index]); |
There was a problem hiding this comment.
Will this mutate the array the user passed into Express, or is this operating on a copy?
| var value = obj[key]; | ||
| // If the value is a BigInt, convert it to a number | ||
| if (typeof value === 'bigint') { | ||
| obj[key] = Number(value); |
There was a problem hiding this comment.
Will this mutate the object that is passed into Express, or is this operating on a copy? May need to be very mindfull bc objects may have setters defined or even the property can have only a getter and no setter at all.
There was a problem hiding this comment.
As per my logic, it is doing mutation,
which one is better, doing mutation or generate a new copy!
I am little confused, if generating a new copy is the better solution, is there any edge case we may lose some properties of the old object in the new object!
even if I do this, i won't recommend using lodash here because it will make package a little more heavy, and clonedeep is slow.
There was a problem hiding this comment.
We don't want to mutute whatever the user is passing in to res.send as that would be very unexpected and can even cause whatever is done with it to change or be unpredictable. For example the app also passed the object to be saved in the db, but then this is changing the values all of a sudden, changing what gets saved in the db -- or a race condition if this if they are both happening async to each other. The object passed to res.send may even be frozen.
There was a problem hiding this comment.
I understood the case, in which when we sent the response first and then saved into DB, it will lose the fully original context,
I should have a copy of this.
dougwilson
commented
Feb 5, 2024
I left some comments and just not sure about this in general, as it seems to me how to seralize big numbers is not really universal, and this PR seems to default to silent data loss. |
bhavya3024
commented
Feb 5, 2024
Let me check this. I cannot add unit test here now because of older nodejs versions but i will definitely add a screenshot here for sure! |
dougwilson
commented
Feb 5, 2024
Just add the tests here, as it definitely won't be landing without tests. |
bhavya3024
commented
Feb 5, 2024
@dougwilson for large numbers,, it represents as an number with expontential like this in the screenshot, whether directly or converting bigint to a number:
|
bhavya3024
commented
Feb 5, 2024
How should I make sure for bigint it won't get blasted in older nodejs versions ? will try to check nodejs version programatically anywhow. |
dougwilson
commented
Feb 5, 2024
@bhavya3024 I think you may perhaps misunderstand how bigints work and how numbers work. You can see that there is a data loss when you take a realistic bigint and covert it to a number:
We really cannot be landing something in Express that would be causing silent data loss. Why would you not be able to simply set Also, your function does not handle circular objects and unlike the original |
bhavya3024
commented
Feb 5, 2024
|
converts bigints to numbers recursively in case of objects/arrays.
Hi there!
I have came across a situation in which we were not able to send responses directly due to bigints, earlier I used to handle this logic in my application code but I thought how about let express handles it automatically, so that new application user doesn't have to handle this on their own.
I have written logic in ES5 intentionally such that it doesn't break very old Nodejs versions if this PR gets merged.
I could have thought to write unit tests for the same, but can you confirm which is the minumum nodejs version should I set (is this progamatically possible btw ?) so that these tests should work only for those nodejs versions which supports BigInt.
@mikeal@peters@nick@mcolyer