Problem with query params

[csbenjamin]

Reproduction

Steps to reproduce the behavior:

When using node-fetch version 2.6.3, making a request to http://nginx/transactions/total?month=2021-09 do not send the url params. If I am using node-fetch version 2.6.1 everything work fine. Enabling node http verbose log, when making the request using node-fetch version 2.6.1 I get:

HTTP 52: createConnection nginx:80: {
  protocol: 'http:',
  slashes: true,
  auth: null,
  host: 'nginx',
  port: 80,
  hostname: 'nginx',
  hash: null,
  search: '?month=2021-09',
  query: 'month=2021-09',
  pathname: '/api/transactions/total',
  path: null,
  href: 'http://nginx/api/transactions/total?month=2021-09',
  method: 'GET',
  headers: [Object: null prototype] {
    Authorization: [
      'Bearer ....'
    ],
    Accept: [ '*/*' ],
    'User-Agent': [ 'node-fetch/1.0 (+https://github.com/bitinn/node-fetch)' ],
    'Accept-Encoding': [ 'gzip,deflate' ],
    Connection: [ 'close' ]
  },
  agent: undefined,
  servername: 'nginx',
  _agentKey: 'nginx:80:'
}

but using node-fetch version 2.6.3 I get

HTTP 24: createConnection nginx:80: {
  path: null,
  pathname: '/api/transactions/total',
  hostname: 'nginx',
  protocol: 'http:',
  port: 80,
  hash: '',
  search: '?month=2021-09',
  query: undefined,
  href: 'http://nginx/api/transactions/total?month=2021-09',
  method: 'GET',
  headers: [Object: null prototype] {
    Authorization: [
      'Bearer ....
    ],
    Accept: [ '*/*' ],
    'User-Agent': [ 'node-fetch/1.0 (+https://github.com/bitinn/node-fetch)' ],
    'Accept-Encoding': [ 'gzip,deflate' ],
    Connection: [ 'close' ]
  },
  agent: undefined,
  host: 'nginx',
  servername: 'nginx',
  _agentKey: 'nginx:80:'
}

Note the query part is undefined when using node-fetch version 2.6.3.

Expected behavior

The behaviour of the two versions should be the same.

Screenshots

Your Environment

software	version
node-fetch	2.6.3
node	14.17.3
npm	6.14.3
Operating System	linux

Additional context

[JasonYeMSFT]

+1 We encountered this bug in our project as well. In the parseURL function, the path property needs to carry the query string since it is so far the only way to send query parameters to the node standard https/http.request API.

[jimmywarting]

Sorry about this, looking into it

[simllll]

We have the same issue, the issue is that whatwgUrl.URL does not return a "query" parametr, only a "search", e.g.:

{
    path: '/youtube/v3/playlistItems',
    pathname: '/youtube/v3/playlistItems',
    hostname: 'youtube.googleapis.com',
    protocol: 'https:',
    port: '',
    hash: '',
    search: '?part=id&part=snippet&part=status&playlistId=PLeLcJvyWxh4jXXCA4jIPH0g6usuZHfnGz&maxResults=50',
    query: undefined, <-- this is undefined, but shouldn't
    href: 'https://youtube.googleapis.com/youtube/v3/playlistItems?part=id&part=snippet&part=status&playlistId=PLeLcJvyWxh4jXXCA4jIPH0g6usuZHfnGz&maxResults=50'
  }

Also interesting is that watwg-url is avaiable in version 9, but the referenced version is ^5.0.0. On my monorepo setup, version 9 is installed though, I guess this could also be related?

ace7536

[pimterry]

v2.6.4 was just released, and fixes this for me 👍

[jimmywarting]

Yea, released it as a hot fix without merging #3001

did not announce it or closed this issue,
#1301 should be merged/accepted

[Exilz]

v2.6.4 fixed this for us too. The latest version broke our app in production 🥲

[LinusU]

Very sorry for the breakage everyone!

A fix has been pushed out as version 2.6.4 and we have added a test case so that this will not break again in the future.

Thank you @jimmywarting for the quick fix ❤️

I would recommend everyone to use package lock files to avoid having your production break, that way you can test your app before any dependencies change.

[jimmywarting]

Already release, closing now as it got merged

[LinusU]

@jimmywarting would you be able to deprecate version 2.6.3 on npm?

[jimmywarting]

@jimmywarting would you be able to deprecate version 2.6.3 on npm?

Something in lines of: npm deprecate node-fetch@2.6.3 "critical bug fixed in v2.6.4"? is that right?

[LinusU]

Sounds good 👍

or maybe:

npm deprecate node-fetch@2.6.3 "query parameters being dropped bug fixed in v2.6.4"

[jimmywarting]

Or after #1303 is merged:

npm deprecate node-fetch@2.6.3 "Query parameters being dropped. Bug fixed in v2.6.5"

[LinusU]

Hmm, that specific bug was fixed in 2.6.4 though, I think that people will just upgrade to the latest version anyways? And I don't think that nearly as many people are affected by #1303, which also manifests as a crash on startup so no subtle behaviour...

But anyways, no strong feelings from my side so you can decide 👍

[LinusU]

Some more info on what and why this happened:

I was back porting #701 to the v2.x branch, specifically this line:

https://github.com/node-fetch/node-fetch/pull/701/files#diff-19dd46c0a2527b1db65f8b69bf935c70e64cd34db0f1d8f3c2fb1b92bf7a467fR270

But what I failed to realise that it had a follow up PR later that fixed the very same bug that I now introduced:

https://github.com/node-fetch/node-fetch/pull/759/files#diff-19dd46c0a2527b1db65f8b69bf935c70e64cd34db0f1d8f3c2fb1b92bf7a467fR263

This is the issue that previously reported this for the 3.x release line: #749

#759 didn't add any test cases, but I think that a good thing to do now is to port the test case added in the hot fix here to the 3.x branch, and to backport all other tests from 3.x to 2.x to further ensure that we don't do any breaking changes...