Skip to content

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,6 @@
"is-plain-obj": "^3.0.0",
"micromatch": "^4.0.2"
},
"peerDependencies": {
"@types/express": "^4.17.13"
},
"peerDependenciesMeta": {
"@types/express": {
"optional": true
}
},
"engines": {
"node": ">=12.0.0"
},
Expand Down
5 changes: 2 additions & 3 deletions src/_handlers.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type * as express from 'express';
import type { Options } from './types';
import type { Request, Response, Options } from './types';
import type * as httpProxy from 'http-proxy';
import { getInstance } from './logger';
const logger = getInstance();
Expand Down Expand Up @@ -53,7 +52,7 @@ export function getHandlers(options: Options) {
return handlers;
}

function defaultErrorHandler(err, req: express.Request, res: express.Response) {
function defaultErrorHandler(err, req: Request, res: Response) {
// Re-throw error. Not recoverable since req & res are empty.
if (!req && !res) {
throw err; // "Error: Must provide a proper URL as target"
Expand Down
4 changes: 2 additions & 2 deletions src/handlers/fix-request-body.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Author

Choose a reason for hiding this comment

The 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;
Expand Down
13 changes: 4 additions & 9 deletions src/http-proxy-middleware.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import type * as https from 'https';
import type * as express from 'express';
import type { Filter, Request, RequestHandler, Response, Options } from './types';
import type { Filter, Request, RequestMiddleware, Response, Options } from './types';
import * as httpProxy from 'http-proxy';
import { createConfig, Config } from './config-factory';
import * as contextMatcher from './context-matcher';
import * as handlers from './_handlers';
import { getArrow, getInstance } from './logger';
import * as PathRewriter from './path-rewriter';
import * as Router from './router';

export class HttpProxyMiddleware {
private logger = getInstance();
private config: Config;
Expand Down Expand Up @@ -35,19 +35,15 @@ export class HttpProxyMiddleware {

// https://github.com/chimurai/http-proxy-middleware/issues/19
// expose function to upgrade externally
(this.middleware as any).upgrade = (req, socket, head) => {
this.middleware.upgrade = (req, socket, head) => {
if (!this.wsInternalSubscribed) {
this.handleUpgrade(req, socket, head);
}
};
}

// https://github.com/Microsoft/TypeScript/wiki/'this'-in-TypeScript#red-flags-for-this
public middleware: RequestHandler = async (
req: Request,
res: Response,
next: express.NextFunction
) => {
public middleware: RequestMiddleware = async (req, res, next) => {
if (this.shouldProxy(this.config.context, req)) {
try {
const activeProxyOptions = await this.prepareProxyRequest(req);
Expand All @@ -58,7 +54,6 @@ export class HttpProxyMiddleware {
} else {
next();
}

/**
* Get the server object to subscribe to server events;
* 'upgrade' for websocket and 'close' for graceful shutdown
Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ export function createProxyMiddleware(context: Filter | Options, options?: Optio

export * from './handlers';

export { Filter, Options, RequestHandler } from './types';
export { Filter, Options, RequestMiddleware } from './types';
32 changes: 26 additions & 6 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@cdaringe cdaringe Mar 3, 2022

Choose a reason for hiding this comment

The 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>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@cdaringe cdaringe Mar 3, 2022

Choose a reason for hiding this comment

The 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, 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

Copy link
Author

@cdaringe cdaringe Mar 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 RequestMiddleware = RequestMiddlewareFunction & {
Copy link
Author

Choose a reason for hiding this comment

The 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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as express from 'express';
import * as request from 'supertest';
import { createProxyMiddleware } from './test-kit';
import { Options } from '../../src/index';
import { createProxyMiddleware } from '../test-kit';
import { Options } from '../../../src/index';

describe('Usage in Express', () => {
let app: express.Express;
Expand Down
35 changes: 35 additions & 0 deletions test/e2e/servers/http.spec.ts
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);
}
});
});
});
});
4 changes: 2 additions & 2 deletions test/e2e/websocket.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as WebSocket from 'ws';
import { Server as WebSocketServer } from 'ws';
import * as getPort from 'get-port';
import { createProxyMiddleware, createApp } from './test-kit';
import type { RequestHandler } from '../../src/types';
import type { RequestMiddleware } from '../../src/types';

/********************************************************************
* - Not possible to use `supertest` to test WebSockets
Expand All @@ -14,7 +14,7 @@ describe('E2E WebSocket proxy', () => {
let proxyServer: http.Server;
let ws: WebSocket;
let wss: WebSocketServer;
let proxyMiddleware: RequestHandler;
let proxyMiddleware: RequestMiddleware;
let WS_SERVER_PORT: number;
let SERVER_PORT: number;

Expand Down
4 changes: 2 additions & 2 deletions test/unit/fix-request-body.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe('fixRequestBody', () => {
jest.spyOn(proxyRequest, 'setHeader');
jest.spyOn(proxyRequest, 'write');

fixRequestBody(proxyRequest, { body: { someField: 'some value' } } as Request);
fixRequestBody(proxyRequest, { body: { someField: 'some value' } } as unknown as Request);

const expectedBody = JSON.stringify({ someField: 'some value' });
expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
Expand All @@ -58,7 +58,7 @@ describe('fixRequestBody', () => {
jest.spyOn(proxyRequest, 'setHeader');
jest.spyOn(proxyRequest, 'write');

fixRequestBody(proxyRequest, { body: { someField: 'some value' } } as Request);
fixRequestBody(proxyRequest, { body: { someField: 'some value' } } as unknown as Request);

const expectedBody = querystring.stringify({ someField: 'some value' });
expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
Expand Down