feat!: drop throwOnError

nodejs · Aug 12, 2024 · 10cefe4 · 10cefe4
1 parent d63afeb
commit 10cefe4
Show file tree

Hide file tree

Showing 8 changed files with 42 additions and 334 deletions.
diff --git a/docs/docs/api/Dispatcher.md b/docs/docs/api/Dispatcher.md
@@ -201,7 +201,6 @@ Returns: `Boolean` - `false` if dispatcher is busy and further dispatch calls wo
 * **upgrade** `string | null` (optional) - Default: `null` - Upgrade the request. Should be used to specify the kind of upgrade i.e. `'Websocket'`.
 * **bodyTimeout** `number | null` (optional) - The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use `0` to disable it entirely. Defaults to 300 seconds.
 * **headersTimeout** `number | null` (optional) - The amount of time, in milliseconds, the parser will wait to receive the complete HTTP headers while not sending the request. Defaults to 300 seconds.
-* **throwOnError** `boolean` (optional) - Default: `false` - Whether Undici should throw an error upon receiving a 4xx or 5xx response from the server.
 * **expectContinue** `boolean` (optional) - Default: `false` - For H2, it appends the expect: 100-continue header, and halts the request body until a 100-continue is received from the remote server
 
 #### Parameter: `DispatchHandler`

diff --git a/lib/api/api-request.js b/lib/api/api-request.js
@@ -5,15 +5,14 @@ const { AsyncResource } = require('node:async_hooks')
 const { Readable } = require('./readable')
 const { InvalidArgumentError, RequestAbortedError } = require('../core/errors')
 const util = require('../core/util')
-const { getResolveErrorBodyCallback } = require('./util')
 
 class RequestHandler extends AsyncResource {
   constructor (opts, callback) {
     if (!opts || typeof opts !== 'object') {
       throw new InvalidArgumentError('invalid opts')
     }
 
-    const { signal, method, opaque, body, onInfo, responseHeaders, throwOnError, highWaterMark } = opts
+    const { signal, method, opaque, body, onInfo, responseHeaders, highWaterMark } = opts
 
     try {
       if (typeof callback !== 'function') {
@@ -54,7 +53,6 @@ class RequestHandler extends AsyncResource {
     this.trailers = {}
     this.context = null
     this.onInfo = onInfo || null
-    this.throwOnError = throwOnError
     this.highWaterMark = highWaterMark
     this.reason = null
     this.removeAbortListener = null
@@ -118,20 +116,14 @@ class RequestHandler extends AsyncResource {
     this.callback = null
     this.res = res
     if (callback !== null) {
-      if (this.throwOnError && statusCode >= 400) {
-        this.runInAsyncScope(getResolveErrorBodyCallback, null,
-          { callback, body: res, contentType, statusCode, statusMessage, headers }
-        )
-      } else {
-        this.runInAsyncScope(callback, null, null, {
-          statusCode,
-          headers,
-          trailers: this.trailers,
-          opaque,
-          body: res,
-          context
-        })
-      }
+      this.runInAsyncScope(callback, null, null, {
+        statusCode,
+        headers,
+        trailers: this.trailers,
+        opaque,
+        body: res,
+        context
+      })
     }
   }
 

diff --git a/lib/api/api-stream.js b/lib/api/api-stream.js
@@ -1,11 +1,10 @@
 'use strict'
 
 const assert = require('node:assert')
-const { finished, PassThrough } = require('node:stream')
+const { finished } = require('node:stream')
 const { AsyncResource } = require('node:async_hooks')
 const { InvalidArgumentError, InvalidReturnValueError } = require('../core/errors')
 const util = require('../core/util')
-const { getResolveErrorBodyCallback } = require('./util')
 const { addSignal, removeSignal } = require('./abort-signal')
 
 class StreamHandler extends AsyncResource {
@@ -14,7 +13,7 @@ class StreamHandler extends AsyncResource {
       throw new InvalidArgumentError('invalid opts')
     }
 
-    const { signal, method, opaque, body, onInfo, responseHeaders, throwOnError } = opts
+    const { signal, method, opaque, body, onInfo, responseHeaders } = opts
 
     try {
       if (typeof callback !== 'function') {
@@ -55,7 +54,6 @@ class StreamHandler extends AsyncResource {
     this.trailers = null
     this.body = body
     this.onInfo = onInfo || null
-    this.throwOnError = throwOnError || false
 
     if (util.isStream(body)) {
       body.on('error', (err) => {
@@ -79,7 +77,7 @@ class StreamHandler extends AsyncResource {
   }
 
   onHeaders (statusCode, rawHeaders, resume, statusMessage) {
-    const { factory, opaque, context, callback, responseHeaders } = this
+    const { factory, opaque, context, responseHeaders } = this
 
     const headers = responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders)
 
@@ -92,55 +90,42 @@ class StreamHandler extends AsyncResource {
 
     this.factory = null
 
-    let res
+    if (factory === null) {
+      return
+    }
 
-    if (this.throwOnError && statusCode >= 400) {
-      const parsedHeaders = responseHeaders === 'raw' ? util.parseHeaders(rawHeaders) : headers
-      const contentType = parsedHeaders['content-type']
-      res = new PassThrough()
+    const res = this.runInAsyncScope(factory, null, {
+      statusCode,
+      headers,
+      opaque,
+      context
+    })
 
-      this.callback = null
-      this.runInAsyncScope(getResolveErrorBodyCallback, null,
-        { callback, body: res, contentType, statusCode, statusMessage, headers }
-      )
-    } else {
-      if (factory === null) {
-        return
-      }
+    if (
+      !res ||
+      typeof res.write !== 'function' ||
+      typeof res.end !== 'function' ||
+      typeof res.on !== 'function'
+    ) {
+      throw new InvalidReturnValueError('expected Writable')
+    }
 
-      res = this.runInAsyncScope(factory, null, {
-        statusCode,
-        headers,
-        opaque,
-        context
-      })
+    // TODO: Avoid finished. It registers an unnecessary amount of listeners.
+    finished(res, { readable: false }, (err) => {
+      const { callback, res, opaque, trailers, abort } = this
 
-      if (
-        !res ||
-        typeof res.write !== 'function' ||
-        typeof res.end !== 'function' ||
-        typeof res.on !== 'function'
-      ) {
-        throw new InvalidReturnValueError('expected Writable')
+      this.res = null
+      if (err || !res.readable) {
+        util.destroy(res, err)
       }
 
-      // TODO: Avoid finished. It registers an unnecessary amount of listeners.
-      finished(res, { readable: false }, (err) => {
-        const { callback, res, opaque, trailers, abort } = this
-
-        this.res = null
-        if (err || !res.readable) {
-          util.destroy(res, err)
-        }
-
-        this.callback = null
-        this.runInAsyncScope(callback, null, err || null, { opaque, trailers })
+      this.callback = null
+      this.runInAsyncScope(callback, null, err || null, { opaque, trailers })
 
-        if (err) {
-          abort()
-        }
-      })
-    }
+      if (err) {
+        abort()
+      }
+    })
 
     res.on('drain', resume)
 

diff --git a/lib/core/request.js b/lib/core/request.js
@@ -40,7 +40,6 @@ class Request {
     headersTimeout,
     bodyTimeout,
     reset,
-    throwOnError,
     expectContinue,
     servername
   }, handler) {
@@ -86,8 +85,6 @@ class Request {
 
     this.bodyTimeout = bodyTimeout
 
-    this.throwOnError = throwOnError === true
-
     this.method = method
 
     this.abort = null

diff --git a/test/client-stream.js b/test/client-stream.js
@@ -832,100 +832,3 @@ test('stream legacy needDrain', async (t) => {
   })
   await t.completed
 })
-
-test('stream throwOnError', async (t) => {
-  t = tspl(t, { plan: 3 })
-
-  const errStatusCode = 500
-  const errMessage = 'Internal Server Error'
-
-  const server = createServer((req, res) => {
-    res.writeHead(errStatusCode, { 'Content-Type': 'text/plain' })
-    res.end(errMessage)
-  })
-  after(() => server.close())
-
-  server.listen(0, async () => {
-    const client = new Client(`http://localhost:${server.address().port}`)
-    after(() => client.close())
-
-    client.stream({
-      path: '/',
-      method: 'GET',
-      throwOnError: true,
-      opaque: new PassThrough()
-    }, ({ opaque: pt }) => {
-      pt.on('data', () => {
-        t.fail()
-      })
-      return pt
-    }, (e) => {
-      t.strictEqual(e.status, errStatusCode)
-      t.strictEqual(e.body, errMessage)
-      t.ok(true, 'end')
-    })
-  })
-
-  await t.completed
-})
-
-test('stream throwOnError, body is bigger than CHUNK_LIMIT', async (t) => {
-  t = tspl(t, { plan: 3 })
-
-  const errStatusCode = 500
-
-  const server = createServer((req, res) => {
-    res.writeHead(errStatusCode, { 'Content-Type': 'text/plain' })
-    res.end(Buffer.alloc(128 * 1024 + 1))
-  })
-  after(() => server.close())
-
-  server.listen(0, async () => {
-    const client = new Client(`http://localhost:${server.address().port}`)
-    after(() => client.close())
-
-    client.stream({
-      path: '/',
-      method: 'GET',
-      throwOnError: true,
-      opaque: new PassThrough()
-    }, ({ opaque: pt }) => {
-      pt.on('data', () => {
-        t.fail()
-      })
-      return pt
-    }, (e) => {
-      t.strictEqual(e.status, errStatusCode)
-      t.strictEqual(e.body, undefined)
-      t.ok(true, 'end')
-    })
-  })
-
-  await t.completed
-})
-
-test('steam throwOnError=true, error on stream', async (t) => {
-  t = tspl(t, { plan: 1 })
-
-  const server = createServer((req, res) => {
-    res.end('asd')
-  })
-  after(() => server.close())
-
-  server.listen(0, async () => {
-    const client = new Client(`http://localhost:${server.address().port}`)
-    after(() => client.close())
-
-    client.stream({
-      path: '/',
-      method: 'GET',
-      throwOnError: true,
-      opaque: new PassThrough()
-    }, () => {
-      throw new Error('asd')
-    }, (e) => {
-      t.strictEqual(e.message, 'asd')
-    })
-  })
-  await t.completed
-})