Set Content-Type to 'application/x-www-form-urlencoded' when body is an instance of url.URLSearchParams

[jkantr]

Seting Content-Type to 'application/x-www-form-urlencoded' when body is an instance of url.URLSearchParams would match spec as per https://fetch.spec.whatwg.org/#concept-bodyinit-extract

As far as I can tell, this is not something that node-fetch currently does.. and it caught me up on what I consider to be a somewhat obscure bug for 30 minutes.

Interestingly (although possibly unrelated), curl sets this by default when using a simple data cli arg (-d foo=bar) as opposed to using text/plain, but I guess we're more interested in the fetch spec than how curl works.

If this is an acceptable proposal, I can get the PR put together in a fairly short time.

Edit: fixed incorrect whatwg link anchor

[jkantr]

Ultimately my rationale here is that pretty much every http client library in node does this in some way or another (usually with some custom option). request uses the 'form' prop, bhttp uses formData, and unirest uses a .form() chain function. I think it's important that node-fetch also has an abstraction that avoids the explicit 'Content-Type': 'application/x-www-form-urlencoded' header be set when it's a pretty common use case. And the only way that can be done and stay in fetch spec I believe is autodetecting an instance of URLSearchParams.

@TimothyGu (sorry to ping, but I know you're extra familiar with URLSearchParams) - I'm trying to figure out if just typeof this.body === 'object' && this.body.constructor.name === 'URLSearchParams' is sufficient here. The biggest advantage of this ofc is that there's no overhead and is backwards compatible.

The clearly 'safer' way to do it would be URLSearchParams.prototype.get.call(this.body, '') - we can just use a synchronous try/catch, as call() will throw if this.body is not a URLSearchParams. The down side, ofc, is we'll need to import url which has only been available since node v7. Since esm doesn't allow conditional imports, we'd have to mix here and use a try/catch with require to keep compatibility with v4+

I just wanted to get the maintainers thoughts before I went down a path that would be outright rejected :)

[TimothyGu]

I agree with the necessity of this feature. The challenge is, as you have found out, detecting whether an object is a URLSearchParams object.

• A simple instanceof check does not work, since the user might be using Node.js' url implementation, or url-search-params, or whatwg-url's upcoming implementation, or some other one. I did outline a solution based on this in Add a fetch option to allow wrappers #229, but it's over-complicated.
• Object.prototype.toString.call(obj) === '[object URLSearchParams]' doesn't work with all implementations as it depends on Symbol.toStringTag (a es2015 feature). Subclasses are also free to override it, though doing so should be rare.
• obj.constructor.name === 'URLSearchParams' might be the best, but it doesn't work for subclassing at all.
• Duck typing obj.append && obj.delete && obj.get && obj.getAll && obj.set is the way chosen for FormData, which has private properties we can use to reliably detect it, but older Headers objects that have a getAll will pass this test too. obj.sort does work as a discriminator, but it's new and might not be implemented uniformly.

[TimothyGu]

On a second thought, I guess we can combine several of those tests to form a conditional that works for many cases:

function isURLSearchParams(obj) {
  // Duck-typing as a necessary condition.
  if (typeof obj !== 'object' ||
      typeof obj.append !== 'function' ||
      typeof obj.delete !== 'function' ||
      typeof obj.get !== 'function' ||
      typeof obj.getAll !== 'function' ||
      typeof obj.has !== 'function' ||
      typeof obj.set !== 'function') {
    return false;
  }

  // Brand-checking and more duck-typing as optional condition.
  return obj.constructor.name === 'URLSearchParams' ||
         Object.prototype.toString.call(obj) === '[object URLSearchParams]' ||
         typeof obj.sort === 'function';
}

This should work for:

• Node.js, whatwg-url's future impl, and all browsers with obj.sort (all circumstances due to obj.sort)
• Newer url-search-params (all circumstances due to obj.sort)
• Older url-search-params w/o obj.sort (no subclassed due to obj.constructor.name)
• All browsers w/o obj.sort (vanilla or subclassed, but not subclassed with toStringTag overriden)

I think that's a sufficient number of cases covered, and we can always add more cases later.

[jkantr]

Tim you are a wealth of resources, as usual.

I'll double check the logic as you've suggested it be implemented and hopefully have a PR with tests and readme updates by mañana. Thanks again for the support!

[WORMSS]

Just so you know isURLSearchParameters(null) will throw an error. Uncaught TypeError: Cannot read properties of null (reading 'append')
Should always check for null after checking for typeof o === 'object'