-
Notifications
You must be signed in to change notification settings - Fork 847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(types): make request & response generic and remove express dependency #478
Conversation
DeepCode's analysis on #9b8a5a found:
Top issues
👉 View analysis in DeepCode’s Dashboard | Configure the bot |
) => { | ||
if (this.shouldProxy(this.config.context, req)) { | ||
try { | ||
const activeProxyOptions = await this.prepareProxyRequest(req); | ||
this.proxy.web(req, res, activeProxyOptions); | ||
} catch (err) { | ||
next(err); | ||
next && next(err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaict next(err)
could lead to bugs as next could be undefined
|
||
export interface RequestHandler extends express.RequestHandler { | ||
export interface RequestHandler< | ||
Request extends http.IncomingMessage = http.IncomingMessage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making type parameters in all exported types optional to save some possibly unnecessary breaking errors like RequestHandler requires two parameters none passed
. More here.
'/proxy', | ||
createProxyMiddleware({ | ||
onError: (_, request, response) => { | ||
// todo, non trivial fix @ts-expect-error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be fixed by sending a PR to @types/connect
to have the following change over here
- use(fn: HandleFunction): Server;
+ use(fn: SimpleHandleFunction): Server;
+ use(fn: NextHandleFunction): Server;
+ use(fn: ErrorHandleFunction): Server;
- use(route: string, fn: HandleFunction): Server;
+ use(route: string, fn: SimpleHandleFunction): Server;
+ use(route: string, fn: NextHandleFunction): Server;
+ use(route: string, fn: ErrorHandleFunction): Server;
...this would make the inference work. There's no other way to fix it on our end.
Now the question is: Are the improvements in this PR worth having http-proxy-middleware to not work with connect properly? I'm okay with all answers although I would push "Yes" because as http-proxy-middleware should not be responsible for type gotchas of connect, nor should it's users be affected.
Ah silly me didn't realize even |
aside from internalizing the types, another possible approach is to add |
I like the intent of this PR, but it does a few things that aren't ideal.
take a peek @ #721. i'm viewing all of the TS complaint issues, ensuring that #721 appeases all complaints/use-cases, and at this point i think it does, with this one perhaps being the exception, due to the feature request of dropping |
I don't know what
I haven't checked it but as long as it solves the problem this PR solves I'm good |
Fixed in #730 Removal of |
So as ya'll might know, http-proxy-middleware is not coupled with express or any other third-party library. But still the types reflect that.
A few problems that pop because of that...
...won't compile when it should
...shouldn't compile when it does and would create a runtime error "Cannot read property 'toString' of undefined" because
originalUrl
is express-specific...won't compile when it should
A project using
http-proxy-middleware
but not havingexpress
installed won't compile unless with--skipLibChecks
becausetypes.ts
importsexpress
The solution to all these problems in making
request
andresponse
generic which consequently would remove express dependencyThis will be a breaking change for the people who...
let middleware = createProxyMiddleware({ ... })
(instead of directly passing touse
so contextual inference works) ANDSo it'll be a breaking change only if ALL these three are fulfilled, otherwise there's no problems.
The easiest way to migrate is to directly pass to
use
. If for any reason the users can't do that, just annotaterequest
and/orresponse
whatever is being used with the library specific types.Aside: It's my first time doing some server side work with TypeScript and I'm not using express and I see this pattern of libraries having their types depend on express, and not adhering to their source code (that doesn't require express exclusive methods & properties). Like
serve-static
that I recently fixed. It's probably because it's easy to use express types so it seamlessly works with express (as it's popular) and get done with it, but let's change that ;)