feat: add forwardError option to enable error forwarding to next middleware (#2259)

* feat: add `forwardError` option to enable error forwarding to next middleware

* feat: add error middleware support in frameworkFactory for improved error handling

* feat: enhance error handling by removing response headers before forwarding errors

* feat: remove error middleware option from frameworkFactory for cleaner middleware setup

* test: update middleware tests to reflect changes in error handling with forwardError option

* test: update middleware tests to handle error forwarding for hapi framework

* feat: implement error forwarding in koa middleware and update tests accordingly

* feat: add error forwarding support in hono middleware and update tests accordingly

* feat: add forwardError option to README with usage example

Author: bjohansebas

README.md

@@ -79,6 +79,7 @@ See [below](#other-servers) for an example of use with fastify.
 |        **[`writeToDisk`](#writetodisk)**        |        `boolean\|Function`        |                    `false`                    | Instructs the module to write files to the configured location on disk as specified in your `webpack` configuration. |
 |   **[`outputFileSystem`](#outputfilesystem)**   |             `Object`              | [`memfs`](https://github.com/streamich/memfs) | Set the default file system which will be used by webpack as primary destination of generated files.                 |
 | **[`modifyResponseData`](#modifyresponsedata)** |            `Function`             |                  `undefined`                  | Allows to set up a callback to change the response data.                                                             |
+|       **[`forwardError`](#forwarderror)**       |             `boolean`             |                    `false`                    | Enable or disable forwarding errors to the next middleware.                                                          |
 
 The middleware accepts an `options` Object. The following is a property reference for the Object.
 
@@ -476,6 +477,35 @@ instance.waitUntilValid(() => {
 });
 ```
 
+### `forwardError`
+
+Type: `boolean`
+Default: `false`
+
+Enable or disable forwarding errors to the next middleware. If `true`, errors will be forwarded to the next middleware, otherwise, they will be handled by `webpack-dev-middleware` and a response will be handled case by case.
+
+This option don't work with hono, koa and hapi, because of the differences in error handling between these frameworks and express.
+
+```js
+const express = require("express");
+const webpack = require("webpack");
+const middleware = require("webpack-dev-middleware");
+
+const compiler = webpack({
+  /* Webpack configuration */
+});
+
+const instance = middleware(compiler, { forwardError: true });
+
+const app = express();
+app.use(instance);
+
+app.use((err, req, res, next) => {
+  console.log(`Error: ${err}`);
+  res.status(500).send("Something broke!");
+});
+```
+
 ## FAQ
 
 ### Avoid blocking requests to non-webpack resources.

src/index.js

@@ -125,6 +125,7 @@ const noop = () => {};
  * @property {boolean=} lastModified options to generate last modified header
  * @property {(boolean | number | string | { maxAge?: number, immutable?: boolean })=} cacheControl options to generate cache headers
  * @property {boolean=} cacheImmutable is cache immutable
+ * @property {boolean=} forwardError forward error to next middleware
  */
 
 /**
@@ -441,6 +442,7 @@ function koaWrapper(compiler, options) {
             ctx.body = stream;
 
             isFinished = true;
+
             resolve();
           };
           /**
@@ -479,6 +481,13 @@ function koaWrapper(compiler, options) {
         },
       );
     } catch (err) {
+      if (options?.forwardError) {
+        await next();
+
+        // need the return for prevent to execute the code below and override the status and body set by user in the next middleware
+        return;
+      }
+
       ctx.status =
         /** @type {Error & { statusCode: number }} */ (err).statusCode ||
         /** @type {Error & { status: number }} */ (err).status ||
@@ -653,6 +662,12 @@ function honoWrapper(compiler, options) {
         },
       );
     } catch (err) {
+      if (options?.forwardError) {
+        await next();
+
+        // need the return for prevent to execute the code below and override the status and body set by user in the next middleware
+        return;
+      }
       context.status(500);
 
       return context.json({ message: /** @type {Error} */ (err).message });

src/middleware.js

@@ -161,8 +161,6 @@ function wrapper(context) {
     }
 
     const acceptedMethods = context.options.methods || ["GET", "HEAD"];
-    // TODO do we need an option here?
-    const forwardError = false;
 
     initState(res);
 
@@ -180,13 +178,24 @@ function wrapper(context) {
      * @returns {Promise<void>}
      */
     async function sendError(message, status, options) {
-      if (forwardError) {
+      if (context.options.forwardError) {
+        if (!getHeadersSent(res)) {
+          const headers = getResponseHeaders(res);
+
+          for (let i = 0; i < headers.length; i++) {
+            removeResponseHeader(res, headers[i]);
+          }
+        }
+
         const error =
           /** @type {Error & { statusCode: number }} */
           (new Error(message));
         error.statusCode = status;
 
         await goNext(error);
+
+        // need the return for prevent to execute the code below and override the status and body set by user in the next middleware
+        return;
       }
 
       const escapeHtml = getEscapeHtml();

src/options.json

@@ -172,6 +172,11 @@
       "description": "Enable or disable setting `Cache-Control: public, max-age=31536000, immutable` response header for immutable assets (i.e. asset with a hash in file name like `image.a4c12bde.jpg`).",
       "link": "https://github.com/webpack/webpack-dev-middleware#cacheimmutable",
       "type": "boolean"
+    },
+    "forwardError": {
+      "description": "Enable or disable forwarding errors to next middleware.",
+      "link": "https://github.com/webpack/webpack-dev-middleware#forwarderrors",
+      "type": "boolean"
     }
   },
   "additionalProperties": false

test/middleware.test.js

@@ -146,7 +146,6 @@ async function frameworkFactory(
           app.use(item);
         }
       }
-
       return [server, req, instance.devMiddleware];
     }
     default: {
@@ -3610,6 +3609,235 @@ describe.each([
         });
       });
 
+      describe("should call the next middleware for finished or errored requests when forwardError is enabled", () => {
+        let compiler;
+
+        const outputPath = path.resolve(
+          __dirname,
+          "./outputs/basic-test-errors-headers-sent",
+        );
+
+        let nextWasCalled = false;
+
+        beforeAll(async () => {
+          compiler = getCompiler({
+            ...webpackConfig,
+            output: {
+              filename: "bundle.js",
+              path: outputPath,
+            },
+          });
+
+          [server, req, instance] = await frameworkFactory(
+            name,
+            framework,
+            compiler,
+            {
+              etag: "weak",
+              lastModified: true,
+              forwardError: true,
+            },
+            {
+              setupMiddlewares: (middlewares) => {
+                if (name === "hapi") {
+                  // There's no such thing as "the next route handler" in hapi. One request is matched to one or no route handlers.
+                } else if (name === "koa") {
+                  middlewares.push(async (ctx) => {
+                    nextWasCalled = true;
+                    ctx.status = 500;
+                    ctx.body = "error";
+                  });
+                } else if (name === "hono") {
+                  middlewares.push(async (ctx) => {
+                    nextWasCalled = true;
+                    ctx.status(500);
+                    return ctx.text("error");
+                  });
+                } else {
+                  middlewares.push((_error, _req, res, _next) => {
+                    nextWasCalled = true;
+                    res.statusCode = 500;
+                    res.end("error");
+                  });
+                }
+
+                return middlewares;
+              },
+            },
+          );
+
+          instance.context.outputFileSystem.mkdirSync(outputPath, {
+            recursive: true,
+          });
+          instance.context.outputFileSystem.writeFileSync(
+            path.resolve(outputPath, "index.html"),
+            "HTML",
+          );
+          instance.context.outputFileSystem.writeFileSync(
+            path.resolve(outputPath, "image.svg"),
+            "svg image",
+          );
+          instance.context.outputFileSystem.writeFileSync(
+            path.resolve(outputPath, "file.text"),
+            "text",
+          );
+
+          const originalMethod =
+            instance.context.outputFileSystem.createReadStream;
+
+          instance.context.outputFileSystem.createReadStream =
+            function createReadStream(...args) {
+              if (args[0].endsWith("image.svg")) {
+                const brokenStream = new this.ReadStream(...args);
+
+                brokenStream._read = function _read() {
+                  const error = new Error("test");
+                  error.code = "ENAMETOOLONG";
+                  this.emit("error", error);
+                  this.end();
+                  this.destroy();
+                };
+
+                return brokenStream;
+              }
+
+              return originalMethod(...args);
+            };
+        });
+
+        beforeEach(() => {
+          nextWasCalled = false;
+        });
+
+        afterAll(async () => {
+          await close(server, instance);
+        });
+
+        it("should work with piping stream", async () => {
+          const response1 = await req.get("/file.text");
+
+          expect(response1.statusCode).toBe(200);
+          expect(nextWasCalled).toBe(false);
+        });
+
+        it('should return the "500" code for requests above root', async () => {
+          const response = await req.get("/public/..%2f../middleware.test.js");
+
+          expect(response.statusCode).toBe(500);
+          if (name !== "hapi") {
+            expect(response.text).toBe("error");
+            expect(nextWasCalled).toBe(true);
+          } else {
+            expect(nextWasCalled).toBe(false);
+          }
+        });
+
+        it('should return the "500" code for the "GET" request to the bundle file with etag and wrong "if-match" header', async () => {
+          const response1 = await req.get("/file.text");
+
+          expect(response1.statusCode).toBe(200);
+          expect(response1.headers.etag).toBeDefined();
+          expect(response1.headers.etag.startsWith("W/")).toBe(true);
+
+          const response2 = await req.get("/file.text").set("if-match", "test");
+
+          expect(response2.statusCode).toBe(500);
+          if (name !== "hapi") {
+            expect(response2.text).toBe("error");
+            expect(nextWasCalled).toBe(true);
+          } else {
+            expect(nextWasCalled).toBe(false);
+          }
+        });
+
+        it('should return the "500" code for the "GET" request with the invalid range header', async () => {
+          const response = await req
+            .get("/file.text")
+            .set("Range", "bytes=9999999-");
+
+          expect(response.statusCode).toBe(500);
+          if (name !== "hapi") {
+            expect(response.text).toBe("error");
+            expect(nextWasCalled).toBe(true);
+          } else {
+            expect(nextWasCalled).toBe(false);
+          }
+        });
+
+        // TODO: why koa and hono don't catch for their error handling when stream emit error?
+        (name === "koa" || name === "hono" ? it.skip : it)(
+          'should return the "500" code for the "GET" request to the "image.svg" file when it throws a reading error',
+          async () => {
+            const response = await req.get("/image.svg");
+
+            // eslint-disable-next-line jest/no-standalone-expect
+            expect(response.statusCode).toBe(500);
+            if (name !== "hapi") {
+              // eslint-disable-next-line jest/no-standalone-expect
+              expect(nextWasCalled).toBe(true);
+            } else {
+              // eslint-disable-next-line jest/no-standalone-expect
+              expect(nextWasCalled).toBe(false);
+            }
+          },
+        );
+
+        it('should return the "200" code for the "HEAD" request to the bundle file', async () => {
+          const response = await req.head("/file.text");
+
+          expect(response.statusCode).toBe(200);
+          expect(response.text).toBeUndefined();
+          expect(nextWasCalled).toBe(false);
+        });
+
+        it('should return the "304" code for the "GET" request to the bundle file with etag and "if-none-match"', async () => {
+          const response1 = await req.get("/file.text");
+
+          expect(response1.statusCode).toBe(200);
+          expect(response1.headers.etag).toBeDefined();
+          expect(response1.headers.etag.startsWith("W/")).toBe(true);
+
+          const response2 = await req
+            .get("/file.text")
+            .set("if-none-match", response1.headers.etag);
+
+          expect(response2.statusCode).toBe(304);
+          expect(response2.headers.etag).toBeDefined();
+          expect(response2.headers.etag.startsWith("W/")).toBe(true);
+
+          const response3 = await req
+            .get("/file.text")
+            .set("if-none-match", response1.headers.etag);
+
+          expect(response3.statusCode).toBe(304);
+          expect(response3.headers.etag).toBeDefined();
+          expect(response3.headers.etag.startsWith("W/")).toBe(true);
+          expect(nextWasCalled).toBe(false);
+        });
+
+        it('should return the "304" code for the "GET" request to the bundle file with lastModified and "if-modified-since" header', async () => {
+          const response1 = await req.get("/file.text");
+
+          expect(response1.statusCode).toBe(200);
+          expect(response1.headers["last-modified"]).toBeDefined();
+
+          const response2 = await req
+            .get("/file.text")
+            .set("if-modified-since", response1.headers["last-modified"]);
+
+          expect(response2.statusCode).toBe(304);
+          expect(response2.headers["last-modified"]).toBeDefined();
+
+          const response3 = await req
+            .get("/file.text")
+            .set("if-modified-since", response2.headers["last-modified"]);
+
+          expect(response3.statusCode).toBe(304);
+          expect(response3.headers["last-modified"]).toBeDefined();
+          expect(nextWasCalled).toBe(false);
+        });
+      });
+
       describe("should fallthrough for not found files", () => {
         let compiler;
 

types/index.d.ts

@@ -94,6 +94,7 @@ export = wdm;
  * @property {boolean=} lastModified options to generate last modified header
  * @property {(boolean | number | string | { maxAge?: number, immutable?: boolean })=} cacheControl options to generate cache headers
  * @property {boolean=} cacheImmutable is cache immutable
+ * @property {boolean=} forwardError forward error to next middleware
  */
 /**
  * @template {IncomingMessage} [RequestInternal=IncomingMessage]
@@ -449,6 +450,10 @@ type Options<
    * is cache immutable
    */
   cacheImmutable?: boolean | undefined;
+  /**
+   * forward error to next middleware
+   */
+  forwardError?: boolean | undefined;
 };
 type Middleware<
   RequestInternal extends IncomingMessage = import("http").IncomingMessage,