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

querystring.stringify does not support BigInt #36080

Closed
kennytm opened this issue Nov 11, 2020 · 2 comments
Closed

querystring.stringify does not support BigInt #36080

kennytm opened this issue Nov 11, 2020 · 2 comments
Labels
feature request Issues that request new features to be added to Node.js. querystring Issues and PRs related to the built-in querystring module.

Comments

@kennytm
Copy link

kennytm commented Nov 11, 2020

Is your feature request related to a problem? Please describe.

> querystring.stringify({a: 12345678901234567n})
'a='

it should print a=12345678901234567

Describe the solution you'd like

support bigint in additional to string, number and boolean.

i.e., in stringifyPrimitive encode any value where typeof v == 'bigint' as '' + v, the same as 'number'.

Describe alternatives you've considered

No.

@ZYSzys ZYSzys added feature request Issues that request new features to be added to Node.js. querystring Issues and PRs related to the built-in querystring module. labels Nov 12, 2020
yashLadha added a commit to yashLadha/node that referenced this issue Nov 13, 2020
Add support for bigint in strgify for querystring method.
This use the same method as the number but escapes the exponential
notation method as bigint does not play well with the numbers. Instead
in case of stringification there is a separate condition clause to
filter out bigint.

Fixes: nodejs#36080
@yashLadha
Copy link
Contributor

yashLadha commented Nov 13, 2020

@kennytm what will be the use case of sending such large numbers in bigint form, ideally one will prefer to use string for such cases instead of bigint. Also when the parsing is applied what should be the value of the key, should it be 12345678901234567 or 12345678901234567n as both are valid options. Can you elaborate on the feature request?

@kennytm
Copy link
Author

kennytm commented Nov 13, 2020

@yashLadha There is a server producing JSON containing numbers recently upgraded to include 64-bit IDs. (The server is not written in JS and they have a well-defined schema so there is no problem handling 64-bit integers in JSON or query). So we used json-bigint to parse the output. Later we used the url module to generate a GET request, which internally used the querystring module to serialize the query. So that 64-bit ID becomes blank as a bigint isn't a number/string/boolean.

Yes we know we can .toString() and that's indeed our current workaround, but that can be said the same for number and boolean.

querystring.parse won't produce numbers so I don't see why parsing is relevant. The serialization should never contain the "n", as non-JS environment needs to process the output of querystring.

RaisinTen added a commit to RaisinTen/node that referenced this issue Dec 14, 2020
@targos targos closed this as completed in 1eb228e Dec 21, 2020
targos pushed a commit that referenced this issue Dec 21, 2020
Fixes: #36080

PR-URL: #36499
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this issue May 1, 2021
Fixes: #36080

PR-URL: #36499
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. querystring Issues and PRs related to the built-in querystring module.
Projects
None yet
3 participants