search

sindresorhus / got

🌐 Human-friendly and powerful HTTP request library for Node.js
MIT License
14.27k stars 935 forks source link

got.stream documentation does not mention returning a Promise #1967

Open meganwalker-ibm opened 2 years ago

meganwalker-ibm commented 2 years ago

Describe the bug

got.stream seems to return an undocumented Promise in some circumstances. This is evident when you pass invalid Options that fail to be validated by got, which then triggers a Rejected Promise to be returned to the user. The Documentation only mentions that got.stream returns a Duplex.

Actual behavior

An undocumented Promise is returned and Rejected due to the incorrect options, this can cause UnhandledPromiseRejectionWarning if the user does not realise that a Promise can be returned from got.stream

(node:44188) UnhandledPromiseRejectionWarning: RequestError: Unexpected option: meaningless
    at Request._destroy (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/core/index.js:478:21)
    at Request.destroy (internal/streams/destroy.js:39:8)
    at Request.flush (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/core/index.js:249:22)
    at lastHandler (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/create.js:37:26)
    at iterateHandlers (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/create.js:49:28)
    at got (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/create.js:69:16)
    at Function.got.stream (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/create.js:169:37)
    at Options.merge (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/core/options.js:406:27)
    at new Options (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/core/options.js:344:26)
    at new Request (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/core/index.js:233:28)
    at got (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/create.js:31:25)
    at Function.got.stream (file:///Users/megan.walker/firefly/firefly-request-helper/node_modules/got/dist/source/create.js:169:37)
    at makeRequest (/Users/megan.walker/firefly/firefly-request-helper/delete-body-test.js:8:32)

...

Expected behavior

Either a - the Promise response is documented for got.stream(), making clear the Request it returns can be a Promise. b - the rejected Promise is caught by got, and is converted into calling the stream's .on('error', <...>) handler

Now we know that it returns a promise in some circumstances despite the user asking for a stream, documenting would be sufficient to resolve this; though I'm not 100% sure if that's the 'right' thing to do, when the user has asked for a stream instead. ...

Code to reproduce

const options = {
  url: 'https://httpbin.org/get',
  meaningless: 'option'
}

const makeRequest = async (options) => {
  const { default: got } = await import('got')
  const ourRequestStream = got.stream(options)
  ourRequestStream.catch(e => {
    console.log('promise error')
  })
  ourRequestStream
    .on('error', (e) => {
      console.log('stream error')
    })
}

makeRequest(options)

This prints out 'promise error'. Failing to make use of the undocumented .catch() function from the Promise means you're likely to get an UnhandledPromiseRejectionWarning unless you have code to catch those more generally. It does not print the 'stream error' line, nor do any event handlers get called.

Checklist

LinusU commented 1 year ago

I've observed this as well. It worked in earlier versions of Got (I think 10.x), but is currently not working without an extra await.

I had this code:

const source = client.stream('https://example.com/')

for await (const chunk of source) {
  console.log(chunk)
}

Which now results in:

TypeError: data is not async iterable

Adding await to the return value of client.stream fixes it:

const source = await client.stream('https://example.com/')

for await (const chunk of source) {
  console.log(chunk)
}