Skip to content

fix: support HTTPS servers and handle named pipe ports #526

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.

Sign up for GitHub

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

Open
rajan-chari wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: support HTTPS servers and handle named pipe ports #526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 34 additions & 1 deletion packages/apps/src/http/express-adapter.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import fs from 'fs';
import http from 'http';
import https from 'https';

import os from 'os';
import path from 'path';
Expand All @@ -10,7 +11,7 @@ import supertest from 'supertest';
import { ExpressAdapter } from './express-adapter';

describe('ExpressAdapter', () => {
let server: http.Server;
let server: http.Server | https.Server;
let adapter: ExpressAdapter;

afterEach(() => {
Expand Down Expand Up @@ -196,4 +197,36 @@ describe('ExpressAdapter', () => {
}
});
});

describe('HTTPS server support', () => {
it('should accept an https.Server and wire it up correctly', async () => {
// https.createServer() without certs creates a valid server object
// that is instanceof https.Server (but NOT instanceof http.Server)
const httpsServer = https.createServer({});

expect(httpsServer instanceof https.Server).toBe(true);
expect(httpsServer instanceof http.Server).toBe(false);

expect(() => {
adapter = new ExpressAdapter(httpsServer);
}).not.toThrow();

// Verify the adapter stored the server: stop() should reject with
// "Server is not running" (not "managed externally" which would mean
// the adapter didn't recognize the https.Server and created its own)
await expect(adapter.stop()).rejects.toThrow('Server is not running');
Comment on lines +214 to +217
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

This assertion is tightly coupled to a specific error message string, making the test brittle to future wording changes. A more robust assertion would directly verify the adapter is using the passed-in server instance (e.g., by checking internal state via a safe test-only accessor, or by asserting behavior that cannot depend on the exact wording, such as checking the error type/shape if available).

Suggested change
// Verify the adapter stored the server: stop() should reject with
// "Server is not running" (not "managed externally" which would mean
// the adapter didn't recognize the https.Server and created its own)
await expect(adapter.stop()).rejects.toThrow('Server is not running');
// Verify the adapter stored the server by asserting stop() rejects in
// this unstarted state, without coupling the test to exact wording.
expect.assertions(2);
try {
await adapter.stop();
} catch (error) {
expect(error).toBeInstanceOf(Error);
expect((error as Error).message).toEqual(expect.any(String));
}

Copilot uses AI. Check for mistakes.
});

it('should register routes on https.Server', () => {
const httpsServer = https.createServer({});
adapter = new ExpressAdapter(httpsServer);

// Should not throw — routes register on the internal Express app
expect(() => {
adapter.registerRoute('POST', '/api/messages', async () => {
return { status: 200, body: { ok: true } };
});
}).not.toThrow();
});
});
});
7 changes: 4 additions & 3 deletions packages/apps/src/http/express-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import http from 'http';
import https from 'https';

import cors from 'cors';
import express from 'express';
Expand Down Expand Up @@ -27,12 +28,12 @@ export class ExpressAdapter implements IHttpServerAdapter {
readonly use: express.Application['use'];

protected express: express.Application;
protected server?: http.Server;
protected server?: http.Server | https.Server;
protected logger: ILogger;
protected onError?: (err: Error) => void;

constructor(serverOrApp?: http.Server | express.Application, options?: { logger?: ILogger; onError?: (err: Error) => void }) {
if (serverOrApp instanceof http.Server) {
constructor(serverOrApp?: http.Server | https.Server | express.Application, options?: { logger?: ILogger; onError?: (err: Error) => void }) {
if (serverOrApp instanceof http.Server || serverOrApp instanceof https.Server) {
// The adapter handles all requests on this server. Use the adapter's
// methods (get, post, use, etc.) to add routes. If you need your own
// Express app, pass it in instead and manage the server yourself.
Expand Down
16 changes: 14 additions & 2 deletions packages/dev/src/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,20 @@ export class DevtoolsPlugin {
}

onStart({ port }: IPluginStartEvent) {
const numericPort = this.options.customPort ?? (
typeof port === 'string' ? parseInt(port, 10) + 1 : port + 1);
let numericPort: number;
if (this.options.customPort) {
numericPort = this.options.customPort;
} else if (typeof port === 'number') {
numericPort = port + 1;
} else {
Comment on lines +106 to +111
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

customPort is checked via truthiness, so a valid value like 0 will be ignored and the code will fall back to deriving a port. Use an explicit null/undefined check instead (e.g., this.options.customPort != null) or check typeof this.options.customPort === 'number'.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@heyitsaamir heyitsaamir Apr 15, 2026

Choose a reason for hiding this comment

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

IMO, this is not worth fixing. Devtools is supposed to be for local development.

const parsed = parseInt(port, 10);
if (isNaN(parsed)) {
numericPort = 3979;
this.log.warn(`Port is a named pipe (${port}), using default devtools port ${numericPort}. Set customPort to override.`);
} else {
numericPort = parsed + 1;
}
Comment on lines +112 to +118
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

parseInt() will accept partially-numeric strings (e.g. "3000abc" => 3000), which can incorrectly treat an invalid PORT as valid. Consider using a stricter parse (e.g., Number(port) with Number.isFinite(...) plus a full-string numeric check) so only purely numeric ports are incremented; otherwise treat it as a named pipe/invalid string and use the fallback.

Copilot uses AI. Check for mistakes.
}

this.express.use(
router({
Expand Down
Loading