refactor: use base node http types#721
Conversation
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.
| * Fix proxied body if bodyParser is involved. | ||
| */ | ||
| export function fixRequestBody(proxyReq: http.ClientRequest, req: http.IncomingMessage): void { | ||
| const requestBody = (req as Request).body; |
There was a problem hiding this comment.
cast no longer required. augment handles this
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
| next: Next | ||
| ) => unknown | Promise<unknown>; | ||
|
|
||
| export type RequestMiddleware = RequestMiddlewareFunction & { |
There was a problem hiding this comment.
and here's the 2nd most significant change 👀
|
Thanks for taking a shot at improving the Types. 🙏 Really like the improvements you've made so far. Could you change the base branch of this PR to v3? 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. |
| req: http.IncomingMessage, | ||
| res: http.ServerResponse, | ||
| next: Next | ||
| ) => unknown | Promise<unknown>; |
There was a problem hiding this comment.
Given that this function is using a callback, should it instead be => void | Promise<void>?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| export type Next = (...args: unknown[]) => unknown; |
There was a problem hiding this comment.
| export type Next = (...args: unknown[]) => unknown; | |
| export type Next = (err?: any) => void; |
There was a problem hiding this comment.
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
|
moved to #723 |
2 participants
Description
http.IncomingMessagewith http-proxy-middleware values permissible onreq, but not present by defaultBREAKING CHANGE:
RequestHandler=>RequestMiddleware. Middleware nowis 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?
Types of changes
Checklist: