Node-fetch: Should we deprecate timeout in favor of AbortController?

[jimmywarting]

Timeout was a node-fetch only additions in it's early days and was never implemented in the spec. Now that there is a way to control it using the AbortController to be able to control when it should abort a request. One could control whether or not a timeout should affect the hole request and response or one or the other

what would you think about removing it and instead help out on creating a abort signal?

(thinking maybe deprecate timeout in v2 or v3 - and remove it completely in v3 or v4)

To help out other on a migration path I where thinking maybe of some way to include a helper function that is optional and isn't included in the bundle so it makes it more modular.

some proposal:

const timeout = require('node-fetch/timeout')
const signal = timeout(1000)
/*
  Same as doing
  const controller = new AbortController()
  const { signal } = controller
  setTimeout(controller.abort, 1000)
*/
fetch(url, { signal })
fetch(url, { signal: timeout(1000) })

Or if it returns a object then you can use spread to append it

const timeout = require('node-fetch/timeout')
console.log( timeout(1000) ) // { signal: signal }

fetch(url, { headers, ...timeout(1000) })

Or returning a object that can help you start and stop the a singal to be able to control if the timeout should apply to only the upload part or the download part

// what node-fetch/timeout would look like
const AbortController = require('./AbortController')

module.exports = function timeout(delay) {
  const controller = new AbortController()
  const { signal } = controller
  let timer = null

  return {
    signal,
    start() {
      clearTimeout(timer)
      timer = setTimeout(controller.abort, delay)
      return signal
    }
    stop() {
      clearTimeout(timer)
    }
  }
} 

const timeout = require('node-fetch/timeout')
const timer = timeout(1000)

// for it's hole duration
fetch(url, { signal: timeout(1000).start() })

// just for upload
const timer = timeout(1000)
fetch(url, { signal: timer.start() })
  .then(res => {
    timer.stop()
  })


// just for download
const timer = timeout(1000)
fetch(url, { signal: timer.signal })
  .then(res => {
    timer.start()
  })

[bitinn]

I think 80-90% people are happy with timeout as it is, so it depends on how easy to use our replacement will be (AbortController isn't the most intuitive thing), if it's indeed as simple as signal: timeout(1000), sure, we might deprecated timeout in future.

[jnields]

@bitinn is the point of this module not to bring the fetch API to the server? If so - this is now part of the fetch API.

I imagine the implementation would go something like this:

fetch = (url, opts) => {
  // ...
  if (opts.signal) signal.addEventListener('abort', () => { cancelRequest() });
  // ...
}

i.e. the AbortController doesn't need to be a part of this package; whatever signal is passed just has to have the same API as AbortSignal.

[bitinn]

now that v2.3.0 is released with AbortSignal support, I think we opened up a path to deprecate timeout in v3.0, where we would likely deprecated support for node < 8.

[ianstormtaylor]

FWIW, I've opened an issue https://github.com/whatwg/fetch/issues/951 with a proposal for a timeout option that solves 90% of use cases, in case anyone's interested. I think it's super important to add to make sure calls to fetch (or res.json()) don't hang indefinitely.