-
Notifications
You must be signed in to change notification settings - Fork 876
refactor: use base node http types #721
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
Changes from all commits
f4595a4
a6ae743
14d5719
54205f0
83dade2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cast no longer required. augment handles this |
||
| export function fixRequestBody(proxyReq: http.ClientRequest, req: Request): void { | ||
| const requestBody = req.body; | ||
|
|
||
| if (!requestBody) { | ||
| return; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,20 +4,40 @@ | |||||
| */ | ||||||
|
|
||||||
| /* eslint-disable @typescript-eslint/no-empty-interface */ | ||||||
|
|
||||||
| import type * as express from 'express'; | ||||||
| import type * as http from 'http'; | ||||||
| import type * as httpProxy from 'http-proxy'; | ||||||
| import type * as net from 'net'; | ||||||
| import type * as url from 'url'; | ||||||
|
|
||||||
| export interface Request extends express.Request {} | ||||||
| export interface Response extends express.Response {} | ||||||
| export type Request = http.IncomingMessage; | ||||||
| export type Response = http.ServerResponse; | ||||||
chimurai marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| export interface RequestHandler extends express.RequestHandler { | ||||||
| upgrade?: (req: Request, socket: net.Socket, head: any) => void; | ||||||
| /** | ||||||
| * http-proxy-middleware supports framework specific values. The following | ||||||
| * values are primarily decorated onto IncomingMessage by express, but are | ||||||
| * not required for use. | ||||||
| */ | ||||||
| declare module 'http' { | ||||||
| interface IncomingMessage { | ||||||
| originalUrl?: string; | ||||||
| hostname?: string; | ||||||
| host?: string; | ||||||
| body?: Record<string, any>; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| export type Next = (...args: unknown[]) => unknown; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
|
|
||||||
| type RequestMiddlewareFunction = ( | ||||||
| req: http.IncomingMessage, | ||||||
| res: http.ServerResponse, | ||||||
| next: Next | ||||||
| ) => unknown | Promise<unknown>; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this function is using a callback, should it instead be
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in other words, if |
||||||
|
|
||||||
| export type RequestMiddleware = RequestMiddlewareFunction & { | ||||||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and here's the 2nd most significant change 👀 |
||||||
| upgrade?: (req: Request, socket: net.Socket, head: any) => void; | ||||||
| }; | ||||||
|
|
||||||
| export type Filter = string | string[] | ((pathname: string, req: Request) => boolean); | ||||||
|
|
||||||
| export interface Options extends httpProxy.ServerOptions { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import * as http from 'http'; | ||
| import { createProxyMiddleware } from '../test-kit'; | ||
| import { Options } from '../../../src/index'; | ||
| import * as request from 'supertest'; | ||
| import * as getPort from 'get-port'; | ||
|
|
||
| describe('http integration', () => { | ||
| it('should work with raw node http RequestHandler', async () => { | ||
| await new Promise(async (resolve, reject) => { | ||
| const port = await getPort(); | ||
| const server = http | ||
| .createServer((req, res) => { | ||
| const proxyConfig: Options = { | ||
| changeOrigin: true, | ||
| logLevel: 'silent', | ||
| target: 'http://jsonplaceholder.typicode.com', | ||
| }; | ||
| const handler = createProxyMiddleware(proxyConfig); | ||
| return handler(req, res, resolve); | ||
| }) | ||
| .listen(port); | ||
| request(server) | ||
| .get('/') | ||
| .expect(200) | ||
| .end((err, res) => { | ||
| if (err) { | ||
| reject(err); | ||
| } else { | ||
| expect(res.ok).toBe(true); | ||
| resolve(res); | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.