refactor: use base node http types #721

cdaringe · 2022-02-27T00:53:26Z

Description

Refactor http-proxy-middleware middeware types to be properly x-server compatible, from both a typescript & node.js lens
Augment http.IncomingMessage with http-proxy-middleware values permissible on req, but not present by default

BREAKING CHANGE: RequestHandler => RequestMiddleware. Middleware now
is expressed in terms of node core http.* types, which express req/res
are compatible with. Orienting to base http.* types allows better typing
across all node servers, as eventually each implementation has these
values in their servers. That is to say, express, koa, fastify, next,
etc all use http.IncomingMessage/http.ServerResponse under the hood,
thus the new middleware types are compatible with everyone.

Technically this should be backwards compatible for most users. However, exotic coupling to the previous types (e.g. Parameters<typeof createProxyMiddleware> could induce subtle breakages.

Motivation and Context

See #719

How has this been tested?

Mo' tests added!

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.

BREAKING CHANGE: `RequestHandler` => `RequestMiddleware`. Middleware now is expressed in terms of node core http.* types, which express req/res are compatible with. Orienting to base http.* types allows better typing accross all node servers, as eventually each implementation has these values in their servers. That is to say, express, koa, fastify, next, etc all use http.IncomingMessage/http.ServerResponse under the hood, thus the new middleware types are compatible with everyone.

cdaringe · 2022-02-27T00:55:48Z

src/handlers/fix-request-body.ts

@@ -5,8 +5,8 @@ import * as querystring from 'querystring';
 /**
 * Fix proxied body if bodyParser is involved.
 */
-export function fixRequestBody(proxyReq: http.ClientRequest, req: http.IncomingMessage): void {
-  const requestBody = (req as Request).body;


cast no longer required. augment handles this

src/http-proxy-middleware.ts

BREAKING CHANGE: `RequestHandler` => `RequestMiddleware`. Middleware now is expressed in terms of node core http.* types, which express req/res are compatible with. Orienting to base http.* types allows better typing accross all node servers, as eventually each implementation has these values in their servers. That is to say, express, koa, fastify, next, etc all use http.IncomingMessage/http.ServerResponse under the hood, thus the new middleware types are compatible with everyone.

…eware into refactor/types

package.json

src/types.ts

cdaringe · 2022-02-27T01:09:35Z

src/types.ts

+  next: Next
+) => unknown | Promise<unknown>;
+
+export type RequestMiddleware = RequestMiddlewareFunction & {


and here's the 2nd most significant change 👀

src/http-proxy-middleware.ts

chimurai · 2022-02-27T12:02:06Z

Thanks for taking a shot at improving the Types. 🙏

Really like the improvements you've made so far.
Looks like your changes are tackling the majority of the typing issues. (as listed in #720)

Could you change the base branch of this PR to v3?
(https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request)

Was already working on some improvements (breaking changes) to free up some legacy code which was preventing future improvements.

Timing is a bit unfortunate. Hopefully it won't create too many conflicts with this PR. 🤞

I'll start reviewing (manually as well) when the base branch is changed to v3.

marchuffnagle · 2022-02-27T14:54:04Z

src/types.ts

+  req: http.IncomingMessage,
+  res: http.ServerResponse,
+  next: Next
+) => unknown | Promise<unknown>;


Given that this function is using a callback, should it instead be => void | Promise<void>?

we could, but I'd suggest that such a type may be less portable. it would force all middleware providers to actually have void return types. i think they generally are void (connect, express, koa), which makes such a suggestion quite reasonable. however, unknown doesn't hurt--as those middleware consumers aren't ever going to consume those values. for middlewares that do return a value, using void means they wouldn't be able to use http-proxy-middleware typed, and i'd like to err on the side of supporting any such users

in other words, if marcs-cool-server used a middleware type of (req,res,next) => res, we'd be in trouble. at the office, i have some cohorts who were quite passionate about res returning middleware, which i didn't necessarily agree with. but know that such impls do exist out there in userspace. perhaps not by the big players.

marchuffnagle · 2022-02-27T14:59:00Z

src/types.ts

 }

+export type Next = (...args: unknown[]) => unknown;


Suggested change

export type Next = (...args: unknown[]) => unknown;

export type Next = (err?: any) => void;

To match https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/express-serve-static-core/index.d.ts#L34

that's an express specific API, which this PR is intentionally trying to decouple from. using that type would assert that param0 is always an optional err in other servers' next interfaces, which may not be the case. for max x-server compat, i'd opt to not offer such a coupled interface, but perhaps allow a user to extend somehow.

i have to refactor this for the v3 branch, i'll think more on it shortly

cdaringe · 2022-03-03T19:04:31Z

moved to #723