Skip to content
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

feat: try other logger ports #3534

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
e245cf5
feat/try other logger ports
DevanceJ Mar 14, 2024
9b8e9e9
made some changes as per the code review and added unit tests for Logger
DevanceJ Mar 15, 2024
6d5fd76
transfered server functions to utils and added tests
DevanceJ Mar 15, 2024
7c630c7
removed old logger tests
DevanceJ Mar 15, 2024
1254b70
changed unit tests and got rid of stub
DevanceJ Mar 18, 2024
feee8fe
creating a server to make the tests more robust
DevanceJ Mar 19, 2024
32809e8
updated logger class according new param passed to start
DevanceJ Mar 19, 2024
a8f6a88
Merge branch 'main' into feat/try-other-ports
BlackHole1 Mar 20, 2024
59d6033
Merge branch 'main' into feat/try-other-ports
BlackHole1 Mar 21, 2024
7df57bf
bumped dependency version of core/utils to 7.3.1
DevanceJ Mar 21, 2024
39e4961
Merge branch 'main' into feat/try-other-ports
DevanceJ Mar 23, 2024
dc4fe2f
Merge branch 'main' into feat/try-other-ports
DevanceJ Apr 2, 2024
012bfe2
Merge branch 'main' into feat/try-other-ports
DevanceJ Apr 2, 2024
f5f9056
Merge branch 'main' into feat/try-other-ports
DevanceJ Apr 6, 2024
c6bc940
Merge branch 'main' into feat/try-other-ports
DevanceJ Apr 13, 2024
c38e6ce
bumped the version to resolve tests
DevanceJ Apr 15, 2024
4aff4c7
Merge branch 'main' into feat/try-other-ports
DevanceJ Apr 20, 2024
caae0a2
refactor portoccupied function and tests
DevanceJ Apr 20, 2024
57275fe
Merge branch 'main' into feat/try-other-ports
DevanceJ Apr 22, 2024
7a44cbb
refactors webpack plugin to avoid any ts errors
DevanceJ Apr 23, 2024
547acf9
Merge branch 'main' into feat/try-other-ports
DevanceJ Apr 28, 2024
bac9781
Merge branch 'main' into feat/try-other-ports
DevanceJ May 2, 2024
86850d9
Merge branch 'main' into feat/try-other-ports
DevanceJ May 4, 2024
2f15245
Merge branch 'main' into feat/try-other-ports
erickzhao May 8, 2024
825a426
fixes ts errors?
DevanceJ May 29, 2024
dc8dacd
Merge branch 'main' into feat/try-other-ports
DevanceJ Jun 7, 2024
d372fb2
Merge branch 'main' into feat/try-other-ports
DevanceJ Jun 11, 2024
ead16ac
Merge branch 'main' into feat/try-other-ports
DevanceJ Jun 18, 2024
5b411b0
Merge branch 'main' into feat/try-other-ports
DevanceJ Jun 23, 2024
fb06062
Merge branch 'main' into feat/try-other-ports
DevanceJ Jul 6, 2024
1cd7bc8
Merge branch 'main' into feat/try-other-ports
DevanceJ Jul 11, 2024
c4ca3b9
reverted API names, removed loggerPort arg and kept this.port in cons…
DevanceJ Jul 19, 2024
eaf5703
Merge branch 'main' into feat/try-other-ports
DevanceJ Jul 19, 2024
63d3e93
Merge branch 'main' into feat/try-other-ports
DevanceJ Aug 9, 2024
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
4 changes: 2 additions & 2 deletions packages/plugin/webpack/src/WebpackPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,7 @@ the generated files). Instead, it is ${JSON.stringify(pj.main)}`);

const logger = new Logger(this.loggerPort);
this.loggers.push(logger);
await logger.start();
const currLoggerPort = await logger.start();
BlackHole1 marked this conversation as resolved.
Show resolved Hide resolved

return {
tasks: [
Expand All @@ -587,7 +587,7 @@ the generated files). Instead, it is ${JSON.stringify(pj.main)}`);
title: 'Launching dev servers for renderer process code',
task: async (_, task) => {
await this.launchRendererDevServers(logger);
task.output = `Output Available: ${chalk.cyan(`http://localhost:${this.loggerPort}`)}\n`;
task.output = `Output Available: ${chalk.cyan(`http://localhost:${currLoggerPort}`)}\n`;
},
options: {
persistentOutput: true,
Expand Down
1 change: 1 addition & 0 deletions packages/utils/core-utils/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export * from './rebuild';
export * from './electron-version';
export * from './yarn-or-npm';
export * from './port';
39 changes: 39 additions & 0 deletions packages/utils/core-utils/src/port.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import * as http from 'http';
/**
BlackHole1 marked this conversation as resolved.
Show resolved Hide resolved
* Check if a port is occupied.
* @returns boolean promise that resolves to true if the port is available, false otherwise.
DevanceJ marked this conversation as resolved.
Show resolved Hide resolved
*/
export const portOccupied = async (port: number): Promise<void> => {
return new Promise<void>((resolve, reject) => {
const server = http.createServer().listen(port);
server.on('listening', () => {
server.close();
resolve();
DevanceJ marked this conversation as resolved.
Show resolved Hide resolved
});

server.on('error', (error) => {
if ('code' in error && (error as NodeJS.ErrnoException).code === 'EADDRINUSE') {
reject(new Error(`port: ${port} is occupied`));
} else {
reject(error);
}
});
});
};
/**
BlackHole1 marked this conversation as resolved.
Show resolved Hide resolved
* Find an available port for web UI.
* @returns the port number.
*/
export const findAvailablePort = async (initialPort: number): Promise<number> => {
const maxPort = initialPort + 10;

for (let p = initialPort; p <= maxPort; p++) {
try {
await portOccupied(p);
return p;
} catch (_err) {
// Pass
}
}
throw new Error(`Could not find an available port between ${initialPort} and ${maxPort}. Please free up a port and try again.`);
};
41 changes: 41 additions & 0 deletions packages/utils/core-utils/test/port_spec.ts
BlackHole1 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { expect } from 'chai';

import { findAvailablePort, portOccupied } from '../src/port';
describe('Port tests', () => {
describe('portOccupied', () => {
it('should resolve to true if the port is available', async () => {
const port = 49152;
const result = await portOccupied(port);
expect(result).to.not.throw;
});

it('should reject if the port is occupied', async () => {
const port = 3000;
try {
await portOccupied(port);
} catch (error) {
expect((error as Error).message).to.equal(`port: ${port} is occupied`);
}
});
});

describe('findAvailablePort', () => {
it('should find an available port', async () => {
const initialPort = 49152;
const port = await findAvailablePort(initialPort);
expect(port).to.be.a('number');
expect(port).to.be.within(initialPort, initialPort + 10);
});

it('should throw an error if no available port is found', async () => {
const initialPort = 3000;
try {
await findAvailablePort(initialPort);
} catch (error) {
expect((error as Error).message).to.equal(
`Could not find an available port between ${initialPort} and ${initialPort + 10}. Please free up a port and try again.`
);
}
});
});
});
BlackHole1 marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions packages/utils/web-multi-logger/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"main": "dist/Logger.js",
"typings": "dist/Logger.d.ts",
"dependencies": {
"@electron-forge/core-utils": "7.3.0",
"express": "^4.17.1",
"express-ws": "^5.0.2",
"xterm": "^4.9.0",
Expand Down
11 changes: 5 additions & 6 deletions packages/utils/web-multi-logger/src/Logger.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import http from 'http';
import path from 'path';

import { findAvailablePort } from '@electron-forge/core-utils';
import express from 'express';
import ews from 'express-ws';

Expand Down Expand Up @@ -33,7 +34,6 @@ export default class Logger {
// I assume this endpoint is just a no-op needed for some reason.
});
}

/**
* Creates a new tab with the given name, the name should be human readable
* it will be used as the tab title in the front end.
Expand All @@ -49,12 +49,11 @@ export default class Logger {
*
* @returns the port number
*/
start(): Promise<number> {
return new Promise<number>((resolve) => {
this.server = this.app.listen(this.port, () => resolve(this.port));
});
async start(): Promise<number> {
this.port = await findAvailablePort(this.port);
this.server = this.app.listen(this.port);
return this.port;
}

/**
* Stop the HTTP server hosting the web UI
*/
Expand Down