docs: add info about serializable types for `querystring.stringify()` by shubheksha · Pull Request #12313 · nodejs/node

shubheksha · 2017-04-10T19:38:04Z

querystring.stringify() doesn't serialize some values. Explicitly mention what values are serialized in the docs.

Fixes #12234

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

querystring.stringify() doesn't serialize some values. Explicitly mention what values are serialized in the docs.

addaleax

LGTM with a typo. Maybe it would be good to mention what happens for other input, though?

addaleax · 2017-04-10T19:39:51Z

doc/api/querystring.md

 The `querystring.stringify()` method produces a URL query string from a
 given `obj` by iterating through the object's "own properties".

+This method searializes the following types of values passed in `obj`: strings,


typo: serializes

Oops, I'll fix the typo.
I'm not sure if I understand it correctly but from what I understood from the linked source code was that an empty string is returned if the param passed if not of the aforementioned types. So should I mention this implementation detail? Not sure how to frame it.

but from what I understood from the linked source code was that an empty string is returned if the param passed if not of the aforementioned types.

That’s how I read the stringify source code, too. How about adding something like Any other input values will be coerced to empty strings?

Sounds good!

Fix typo. Add what happens when another data type is passed to `querystring.stringify()` besides the ones that can be correctly parsed

hiroppy · 2017-04-10T21:21:48Z

doc/api/querystring.md


+This method serializes the following types of values passed in `obj`: strings,
+finite numbers, booleans and arrays of the aforementioned types.
+Any other input values will be coerced to empty strings


I think that it is better to use the period(I'm sorry if I'm wrong because I am not native).

You're right, thanks for pointing it out :)

joyeecheung · 2017-04-11T03:56:24Z

doc/api/querystring.md

 The `querystring.stringify()` method produces a URL query string from a
 given `obj` by iterating through the object's "own properties".

+This method serializes the following types of values passed in `obj`: strings,


Maybe this paragragh can be added to the type annotation of obj above? Using the {Type} syntax would give us links to MDN documentations. Something like

Values should be {String|Number|Boolean|String[]|Number[]|Boolean[]} (more explanations)

Feel free to ignore, but I would just drop the This method at the front :-)

@joyeecheung, this is neat -- I didn't know this!
@jasnell, dropped :)

jasnell · 2017-04-11T08:37:55Z

doc/api/querystring.md

 The `querystring.stringify()` method produces a URL query string from a
 given `obj` by iterating through the object's "own properties".

+This method serializes the following types of values passed in `obj`: strings,


Feel free to ignore, but I would just drop the This method at the front :-)

lpinca · 2017-04-12T19:10:36Z

doc/api/querystring.md

 given `obj` by iterating through the object's "own properties".

+It serializes the following types of values passed in `obj`:
+{String|Number|Boolean|String[]|Number[]|Boolean[]}


Nit: if I'm not wrong primitive types should be in lowercase.

querystring.stringify() doesn't serialize some values. Explicitly mention what values are serialized in the docs. Add what happens when another data type is passed to `querystring.stringify()` besides the ones that can be correctly parsed PR-URL: nodejs#12313 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2017-04-14T21:09:17Z

Landed in b07c72b, fixed @lpinca’s nit while landing

querystring.stringify() doesn't serialize some values. Explicitly mention what values are serialized in the docs. Add what happens when another data type is passed to `querystring.stringify()` besides the ones that can be correctly parsed PR-URL: #12313 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

querystring.stringify() doesn't serialize some values. Explicitly mention what values are serialized in the docs. Add what happens when another data type is passed to `querystring.stringify()` besides the ones that can be correctly parsed PR-URL: nodejs/node#12313 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

docs: add info about serializable types

3ed323f

querystring.stringify() doesn't serialize some values. Explicitly mention what values are serialized in the docs.

nodejs-github-bot added doc Issues and PRs related to the documentations. querystring Issues and PRs related to the built-in querystring module. labels Apr 10, 2017

addaleax approved these changes Apr 10, 2017

View reviewed changes

doc: add info about other data types

31fca1c

Fix typo. Add what happens when another data type is passed to `querystring.stringify()` besides the ones that can be correctly parsed

hiroppy reviewed Apr 10, 2017

View reviewed changes

joyeecheung reviewed Apr 11, 2017

View reviewed changes

jasnell approved these changes Apr 11, 2017

View reviewed changes

doc: uses {} to link to mdn docs for types

fec24ec

jasnell approved these changes Apr 12, 2017

View reviewed changes

lpinca approved these changes Apr 12, 2017

View reviewed changes

lpinca reviewed Apr 12, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:12

addaleax closed this Apr 14, 2017

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

MylesBorins added the land-on-v6.x label May 15, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

refack mentioned this pull request Jul 19, 2017

Any way to bypass querystring.stringify with empty Object ? #12234

Closed

Uh oh!

Conversation

shubheksha commented Apr 10, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiroppy Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Apr 11, 2017 • edited by TimothyGu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hiroppy Apr 10, 2017 •

edited

Loading

joyeecheung Apr 11, 2017 •

edited by TimothyGu

Loading