Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

url.format/querystring.stringify include undefined parameters in querystring #16524

Closed
punmechanic opened this issue Apr 18, 2015 · 1 comment

Comments

@punmechanic
Copy link

url.format (on node 0.12.0, which in turn calls querystring.stringify) will include parameters on an object that are explicitly undefined. If you don't want to read the real life consequences, here's some example code demonstrating the issue - just skip to the bottom.

Dan@DAN-PC ~
$ node
> var querystring = require('querystring');
undefined
> querystring.stringify({foo:undefined})
'foo='

Using an excerpt from the library I am developing:

  function all(region, freeToPlayOnly) {
    return riot.get(region, version, 'champion', {
      freeToPlay: freeToPlayOnly
    });
  }

If I do not pass in freeToPlayOnly to all(), then obviously it will be undefined. The object passed to riot.get will, therefore, be { freeToPlay: undefined }. Inside riot.get, the following code is invoked:

  function get(region, version, path, queryParameters) {
    var myUrl = url.format({
      host: util.format(base, region),
      protocol: 'http',
      pathname: util.format(pathFormat, region, version, path),
      query: queryParameters
    });

    // the following is example code
    return http.get(myUrl);
  }

As you can see from the above code, our { freeToPlay: undefined } is passed to the get function as queryParameters, which are then passed to query.

If you invoke this method and examine myUrl, it will be host/pathname?freeToPlay=. In my opinion, this is incorrect - I would have expected to have received host/pathname back (as the value of freeToPlay is undefined). The current behaviour would make sense if I explicitly passed null to the url.format method, but I did not.

Hope i've explained this well enough.

@punmechanic punmechanic changed the title url.format includes undefined parameters in querystring url.format/querystring.stringify include undefined parameters in querystring Apr 18, 2015
@cjihrig
Copy link

cjihrig commented Apr 22, 2015

FWIW, this has been proposed and rejected before (see #7971).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants