refactor: use base node http types #723

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

Description

Re-post of #721 against v3

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.

…eware into refactor/types

…chimurai#722)

cdaringe · 2022-03-12T07:03:15Z

@chimurai , any issues, or g2g? saw tasks were cancelled, but tests were lookin good in CI

chimurai · 2022-03-12T09:44:37Z

Hi @cdaringe.

The tests were running indefinitely in github-actions. I cancelled them after a while. Think it's because the server is still running after the test finished. (just found out I had to submit the review :) )
Re-ran the test. You'll see it's running for more than 30minutes. While they should finish in less than a minute. (https://github.com/chimurai/http-proxy-middleware/actions/runs/1929895626)

Haven't got time yet to do a thorough review. I'll try see if I can do it soon

test/e2e/servers/http.spec.ts

cdaringe · 2022-03-12T19:50:41Z

hmm, odd. locally, they all exit fine. i sent a patch w/ server.close. lets see if it takes. i thought you had cancelled them for other reasons 😆

coveralls · 2022-03-12T20:05:22Z

Coverage remained the same at 98.795% when pulling 1ddcd6b on cdaringe:refactor/types-v3 into 0b30c5d on chimurai:v3.

chimurai · 2022-03-12T20:51:24Z

src/types.ts

+ * values are primarily decorated onto IncomingMessage by express, but are
+ * not required for use.
+ */
+declare module 'http' {


Augmenting http.IncomingMessage does some assumptions on properties from certain frameworks; ie. express.

Wondering if there is a way to provide better support (without hardcoding them?)

I’m AFK for a day or so, but iirc, HPM mutates and/or attempts to read these values. This augment was only to satisfy HPMs existing internal usage of these fields. It does not require frameworks to implement these, luckily.

Will have to visit on a field by field basis. Probably worth going thru them and seeing if we can avoid

Alright, I had some time this morning.

I was almost able to decouple full from express, and improve typesafety (by reducing casts).

cdaringe#1

Unfortunately, there is one macro barrier--and that is that fact that internally express mutates req.url in a fn called trim_prefix and exposes the desired url on originalUrl. this is harmful behavior, and make supporting express difficult on a plain node http, particularly because apps can app.use(...) at many levels inside of express.

so, we need to either need to augment http.IncomingMessage as done above and continue to support originalUrl, or, update the HPM createProxyMiddleware inferface & accept a pathPrefix (🤢 ) and use that to reconstruct original req.urls. The latter sounds dicey, and perhaps has edge cases.

I also posted to express here: expressjs/express#4854

Did some testing today.

Think augmenting native types is somewhat undesirable; As these augmented types will bubble up to the projects in which http-proxy-middleware is used.

Wouldn't adding back @types/express as devDependency makes sense? And cast internal req objects to express.Request at places where it's needed. Instead of creating and maintaining them as internal types. This way it's also documented for which framework it's serving its purpose.

Think augmenting native types is somewhat undesirable; As these augmented types will bubble up to the projects in which http-proxy-middleware is used.

Agreed. Let's not do that.

Wouldn't adding back @types/express as devDependency makes sense? And cast internal req objects to express.Request at places where it's needed.

Ya, we can do that

mmm, in retrospect, if we bring them back and cast, they'd need to be a peer dependency or top level dependency for TS users who aren't skipLibCheck: true'ing :).

the only fields that are needed are body & originalUrl--pretty minimal. to prevent users from having to deal with library and dependency hoopla, i was able to put in minimal types, right around the runtime places where we need to handle express.js special cases as well. thus, we can co-locate express compile & runtime special cases simultaneously, adjacent to one another, with no dependency complexity.

let me annotate my new patches to draw attention to this to you--see if you're game

Getting into a rabbit-hole... Didn't expect changes/refactoring of the internals. 😬
Need some time to review and see if it's going into the right direction.
It's a tough cookie...

Didn't expect changes/refactoring of the internals

ya... sorry about that. it's much less evil than it looks! mainly just a single param drop and halt of req.url usage (both simple!), which generated a ton of 🔴 / 🟢 in the diffs. nonetheless, it was all still on topic for de-expressing--promise! 😬 🙏

refactor: decouple from express

jest.setup.js

cdaringe · 2022-03-13T20:49:13Z

src/_handlers.ts

 import type * as httpProxy from 'http-proxy';
 import { getInstance } from './logger';
+import { getUrl } from './url';


👀 as mentioned in issue discussions, we now have clarity that we cannot just use req.url--it's mutated from express. thus, from the Request type, I have now dropped url: string, as it's unsafe to access in the codebase. getUrl is a safe, express & IncomingMessage compatible accessor

cdaringe · 2022-03-13T20:50:14Z

src/handlers/fix-request-body.ts

-export function fixRequestBody(proxyReq: http.ClientRequest, req: http.IncomingMessage): void {
-  const requestBody = (req as Request).body;
+export function fixRequestBody(proxyReq: http.ClientRequest, req: Request): void {
+  const requestBody = (req as BodyParserRequest).body;


here's an express runtime workaround, with an express type cast, all localized to the module of interest

cdaringe · 2022-03-13T20:53:21Z

src/path-filter.ts

-    return pathFilter(pathname, req);
+export function matchPathFilter(pathFilter: Filter = '/', req: Request): boolean {
+  const url = getUrl(req);
+  switch (typeof pathFilter) {


it's not that I love switch vs the prior ifs, it's rather that the switch narrows pathFilter's many types into narrow types in each block

cdaringe · 2022-03-13T20:54:32Z

src/types.ts

+ *
+ * Use getUrl(req) to get a x-server safe URL.
+ */
+type ExpressCompatibleIncomingMessage = Omit<http.IncomingMessage, 'url'>;


👀 . Here's a big change since our last review. Dropping url out of IncomingMessage, as it's unsafe to reference anywhere due to express support

cdaringe · 2022-03-13T20:57:39Z

src/path-filter.ts

-  if (typeof pathFilter === 'function') {
-    const pathname = getUrlPathName(uri);
-    return pathFilter(pathname, req);
+export function matchPathFilter(pathFilter: Filter = '/', req: Request): boolean {


this is the 2nd biggest change with this PR since we last reviewed. uri: string was dropped as a param. now, url is always extract from the req param, safely.

a nice little bit of simplification 🧹 . this was done as part of the de-express-ificationthe

cdaringe · 2022-03-15T22:30:21Z

Perhaps for a different issue, but directly related to this PR's refactors.

I was speaking w/ dougwilson over in express, and he made a pretty good callout here, that use of originalUrl is likely entirely incorrect in HPM.

That is, HPM shouldn't handle the express special pathname case(s) it handles now at all.

Currently, HPM+express does this:

app.use('/api', proxy('http://localhost/')) 
// then, GET /api/foo => http://localhost/api/foo

but it actually should be:

app.use('/api', proxy('http://localhost/')) 
// then, GET /api/foo => http://localhost/foo

If someone wants the original behavior, then they needed to register the HPM middleware earlier in their app middlewares, and not in the nested app mount. If they wanted something different, then they'd need to use one of HPMs many awesome rewrite APIs.

I think for v3, we ought take heed of this warning (from the express maintainer, no less 😄 ). It additionally simplifies a bunch of the tomfoolery i've done here!

If agreeable, I'd refactor everything accordingly, possibly in a downstream PR, or here.

chimurai · 2022-03-16T00:15:30Z

The originalUrl you've found is exactly the rabbit hole, which I was referring to previously. (You're the first to spot it 😅)
This faulty implementation should be completely removed. (Good to know /dougwilson confirmed the incorrect usage. Thanks for reaching out to him)

Think too many things are happening in #723.

To make it more manageable I would suggest to break it apart:

provide better x-server compatibility
plugin system
move functionality to plugin system (pathRewrite)
remove originalUrl usage and express types?

These were actually things I already started to work on in v3 to allow better compatibility and flexibility.

To provide better x-server compatibility; I created a new branch #730 and cherry picked the changes you made with the focus on improving the base types so you can use in native http server or other express-like servers. (Added you as co-author 🙏). Let me know if this suites your original need in #719.

After that I can continue to work on the plugin system. And clean up the internals. (should make removing originalUrl bit easier)
Bit unfortunate timing... If you start refactoring now, I'm afraid we'll get a lot of conflicts 😬

Removing originalUrl will change how you'll create and mount the proxy, configure it, pathRewriting. But also how can you mount it to sub-routes. Think how WebSockets will be proxied needs to be investigated. This seemingly tiny change will need quite some testing...

About:

This is correct.

app.use('/api', proxy('http://localhost/')) 
// then, GET /api/foo => http://localhost/foo

Without originalUrl changing the basePath will be possible as well with:

app.use('/api', proxy('http://localhost/bar')) 
// then, GET /api/foo => http://localhost/bar/foo

cdaringe · 2022-03-16T22:24:43Z

I agree, there's lot's going on. It is entirely focused on 1 & 4, sans the jest patch. I'm picking up that you have some stuff in flight you wanna do w/ v3, that perhaps intersects with this work? That's fine. I'm probably at this point not going to send four PRs as you've suggested above. for me, i'd vote for one of the the following:

Option 1: we close this PR, and it can just sit here as reference as you noodle thru your v3 plans. i feel like the types are in a good position as they stand here. additionally, as mentioned earlier, some of the thrash is really just to sustain backwards compat w/ expreess stuff and still have good typescript hygiene. with the deletion of that feature, a lot of that diff thrash disappears out of refactor: use base node http types #723
Option 2: accept this PR, and iterate on those other goals. i'm happy to even jump on a call to walk thru this changeset together. v3 is unreleased at this point, so perhaps there's little harm in taking a slightly above average sized changeset.

i wont be upset if option #1 is the pick :)

chimurai · 2022-03-19T16:23:06Z

You sensed it right. Had some plans in mind related to #2 and #3, which hopefully will answers a lot of issues and questions in the issue tracker. Since it's a major version, it's a nice opportunity to do some clean up as well.

If you don't mind, I'll continue with option #1 to keep the changes small and logical and create separate PRs to remove incorrect usage of originalUrl and maybe remove the @express/types dependency.

Really appreciate the time and effort you've put into it. 🙏
Want to thank you for helping out with the improvements to the types and more. (Was struggling to get the types right)
Wish there were more developers like you who aren't afraid to get their hands dirty. :)

chimurai · 2022-03-20T13:07:43Z

Closing PR. Superseded by #730

Will create separated PR to fix the originalUrl issue (#723 (comment)) and investigate removal of the @types/express dependency.

Thanks for helping out! 💪

chimurai and others added 8 commits February 19, 2022 23:38

feat: remove shorthand usage [BREAKING CHANGE] (chimurai#716)

1540dac

Merge branch 'refactor/types' of github.com:cdaringe/http-proxy-middl…

14d5719

…eware into refactor/types

refactor: drop coupling on @types/express

54205f0

fix: drop final express usage in src/*

83dade2

feat(option): refactor context to pathFilter option [BREAKING CHANGE] (…

0b30c5d

…chimurai#722)

Merge branch 'refactor/types' into refactor/types-v3

dee41a7

cdaringe mentioned this pull request Mar 3, 2022

refactor: use base node http types #721

Closed

5 tasks

chimurai requested changes Mar 12, 2022

View reviewed changes

test/e2e/servers/http.spec.ts Outdated Show resolved Hide resolved

fix: close server

1ddcd6b

chimurai reviewed Mar 12, 2022

View reviewed changes

cdaringe mentioned this pull request Mar 13, 2022

RFC: drop mutation of IncomingMessage owned fields, write only to new, augmented fields expressjs/express#4854

Closed

cdaringe and others added 2 commits March 13, 2022 13:43

refactor: decouple from express

878986a

Merge pull request #1 from cdaringe/refactor/types-v3-de-express-ify

c6d6547

refactor: decouple from express

cdaringe commented Mar 13, 2022

View reviewed changes

jest.setup.js Show resolved Hide resolved

cdaringe commented Mar 13, 2022

View reviewed changes

docs: fix comment node assoc

5460cb3

cdaringe commented Mar 13, 2022

View reviewed changes

chore: restore prior middleware setup

4a8bb87

cdaringe commented Mar 13, 2022

View reviewed changes

chimurai force-pushed the v3 branch from c8e0f01 to e4fa0f3 Compare March 20, 2022 12:39

chimurai mentioned this pull request Mar 20, 2022

refactor: use node http base types [BREAKING CHANGE] #730

Merged

5 tasks

chimurai closed this Mar 20, 2022

chimurai mentioned this pull request Mar 20, 2022

fix: server mounting [BREAKING CHANGE] #731

Merged

5 tasks

chimurai mentioned this pull request Feb 22, 2023

feat(typescript): type improvements #882

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use base node http types #723

refactor: use base node http types #723

cdaringe commented Mar 3, 2022

cdaringe commented Mar 12, 2022

chimurai commented Mar 12, 2022 •

edited

Loading

cdaringe commented Mar 12, 2022

coveralls commented Mar 12, 2022

chimurai Mar 12, 2022

cdaringe Mar 12, 2022

cdaringe Mar 13, 2022 •

edited

Loading

cdaringe Mar 13, 2022

chimurai Mar 13, 2022 •

edited

Loading

cdaringe Mar 13, 2022

cdaringe Mar 13, 2022

chimurai Mar 14, 2022

chimurai Mar 14, 2022

cdaringe Mar 14, 2022 •

edited

Loading

cdaringe Mar 13, 2022 •

edited

Loading

cdaringe Mar 13, 2022

cdaringe Mar 13, 2022

cdaringe Mar 13, 2022

cdaringe Mar 13, 2022

cdaringe commented Mar 15, 2022 •

edited

Loading

chimurai commented Mar 16, 2022

cdaringe commented Mar 16, 2022

chimurai commented Mar 19, 2022

chimurai commented Mar 20, 2022

refactor: use base node http types #723

refactor: use base node http types #723

Conversation

cdaringe commented Mar 3, 2022

Description

Motivation and Context

How has this been tested?

Types of changes

Checklist:

cdaringe commented Mar 12, 2022

chimurai commented Mar 12, 2022 • edited Loading

cdaringe commented Mar 12, 2022

coveralls commented Mar 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdaringe Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chimurai Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdaringe Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

cdaringe Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdaringe commented Mar 15, 2022 • edited Loading

chimurai commented Mar 16, 2022

cdaringe commented Mar 16, 2022

chimurai commented Mar 19, 2022

chimurai commented Mar 20, 2022

chimurai commented Mar 12, 2022 •

edited

Loading

cdaringe Mar 13, 2022 •

edited

Loading

chimurai Mar 13, 2022 •

edited

Loading

cdaringe Mar 14, 2022 •

edited

Loading

cdaringe Mar 13, 2022 •

edited

Loading

cdaringe commented Mar 15, 2022 •

edited

Loading