diff --git a/README.md b/README.md index 113fdc2e5..46a54691c 100644 --- a/README.md +++ b/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. diff --git a/src/index.js b/src/index.js index de1403f83..9e7cff79e 100644 --- a/src/index.js +++ b/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 }); diff --git a/src/middleware.js b/src/middleware.js index 078bbead7..946d4d73f 100644 --- a/src/middleware.js +++ b/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} */ 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(); diff --git a/src/options.json b/src/options.json index 0a55b69c9..1e83adaa1 100644 --- a/src/options.json +++ b/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 diff --git a/test/middleware.test.js b/test/middleware.test.js index 6ba82fcd3..0ad0a2740 100644 --- a/test/middleware.test.js +++ b/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; diff --git a/types/index.d.ts b/types/index.d.ts index 1ba81cd6c..e3fb18505 100644 --- a/types/index.d.ts +++ b/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,