feat: refactor with typescript to support esm and cjs both #51
Conversation
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/command-line-test@1.0.10, npm/egg-bin@5.18.1, npm/eslint-config-egg@12.3.1, npm/git-contributor@1.1.0, npm/mm@2.5.0, npm/pedding@1.1.0, npm/power-assert@1.6.1 |
|
想问下 waitPort 方法的使用场景是什么？ |
WalkthroughThe recent updates enhance the project's configuration, dependencies, and functionality. ESLint rules are refined, Node.js versions in workflows are updated, and the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
一直在尝试等待指定端口可用。 |
|
我重新基于你的分支 rebase 一下. |
There was a problem hiding this comment.
Actionable comments posted: 13
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- .eslintrc (1 hunks)
- .github/workflows/nodejs.yml (1 hunks)
- .gitignore (1 hunks)
- bin/detect-port.ts (3 hunks)
- package.json (2 hunks)
- src/detect-port.ts (5 hunks)
- src/index.ts (1 hunks)
- src/wait-port.ts (1 hunks)
- test/cli.test.ts (1 hunks)
- test/detect-port.test.ts (1 hunks)
- test/wait-port.test.ts (1 hunks)
- tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (5)
- .eslintrc
- .github/workflows/nodejs.yml
- .gitignore
- src/index.ts
- tsconfig.json
Additional Context Used
Biome (29)
bin/detect-port.ts (2)
24-24: Use Number.parseInt instead of the equivalent global.
42-42: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
src/detect-port.ts (15)
49-49: Unexpected any. Specify a different type.
64-64: Unexpected any. Specify a different type.
94-94: Unexpected any. Specify a different type.
112-112: Unexpected any. Specify a different type.
119-119: Unexpected any. Specify a different type.
24-24: Reassigning a function parameter is confusing.
28-28: Reassigning a function parameter is confusing.
32-32: Reassigning a function parameter is confusing.
23-23: Reassigning a function parameter is confusing.
27-27: Reassigning a function parameter is confusing.
32-32: Use Number.parseInt instead of the equivalent global.
51-51: Reassigning a function parameter is confusing.
54-54: Reassigning a function parameter is confusing.
55-55: Reassigning a function parameter is confusing.
128-128: Reassigning a function parameter is confusing.
src/wait-port.ts (3)
21-21: Unexpected any. Specify a different type.
22-22: Unexpected any. Specify a different type.
14-14: Use Number.Infinity instead of the equivalent global.
test/cli.test.ts (3)
49-49: Template literals are preferred over string concatenation.
43-43: Use Number.parseInt instead of the equivalent global.
50-50: Use Number.parseInt instead of the equivalent global.
test/detect-port.test.ts (4)
51-51: Prefer for...of instead of forEach.
84-84: Unexpected any. Specify a different type.
90-90: Unexpected any. Specify a different type.
103-103: Template literals are preferred over string concatenation.
test/wait-port.test.ts (2)
11-11: Prefer for...of instead of forEach.
28-28: Unexpected any. Specify a different type.
Additional comments not posted (1)
bin/detect-port.ts (1)
Line range hint
42-42: ReplaceisNaNwithNumber.isNaNfor safer type checking.
[REFACTOR_SUGGESTion]- } else if (isNaN(port)) { + } else if (Number.isNaN(port)) {
| t.mock.reset(); | ||
| }); | ||
|
|
||
| it('work with listening next port 4001 because 4000 was listened to ' + ip(), async () => { |
There was a problem hiding this comment.
Use template literals for string operations to enhance readability and maintainability.
- it('work with listening next port 4001 because 4000 was listened to ' + ip(), async () => {
+ it(`work with listening next port 4001 because 4000 was listened to ${ip()}`, async () => {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| it('work with listening next port 4001 because 4000 was listened to ' + ip(), async () => { | |
| it(`work with listening next port 4001 because 4000 was listened to ${ip()}`, async () => { |
| } | ||
| }); | ||
| after(() => { | ||
| servers.forEach(server => server.close()); |
There was a problem hiding this comment.
Consider using for...of instead of forEach for better readability and performance in modern JavaScript.
- servers.forEach(server => server.close());
+ for (const server of servers) {
+ server.close();
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| servers.forEach(server => server.close()); | |
| for (const server of servers) { | |
| server.close(); | |
| } |
| }); | ||
|
|
||
| it('should listen next port 4001 when localhost is not binding', async t => { | ||
| t.mock.method(dns, 'lookup', (address: string, callback: (...args: any[]) => void) => { |
There was a problem hiding this comment.
Refine type definitions to avoid using any.
- t.mock.method(dns, 'lookup', (address: string, callback: (...args: any[]) => void) => {
+ t.mock.method(dns, 'lookup', (address: string, callback: (...args: unknown[]) => void) => {
- const err = new Error(`getaddrinfo ENOTFOUND ${address}`);
- (err as any).code = 'ENOTFOUND';
+ const err: NodeJS.ErrnoException = new Error(`getaddrinfo ENOTFOUND ${address}`);
+ err.code = 'ENOTFOUND';Also applies to: 90-90
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| t.mock.method(dns, 'lookup', (address: string, callback: (...args: any[]) => void) => { | |
| t.mock.method(dns, 'lookup', (address: string, callback: (...args: unknown[]) => void) => { | |
| const err: NodeJS.ErrnoException = new Error(`getaddrinfo ENOTFOUND ${address}`); | |
| err.code = 'ENOTFOUND'; |
| try { | ||
| const port = 9093; | ||
| await waitPort(port, { retries: 3, retryInterval: 100 }); | ||
| } catch (err:any) { |
There was a problem hiding this comment.
Specify a more precise type than any for error handling.
- } catch (err:any) {
+ } catch (err: Error) {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| } catch (err:any) { | |
| } catch (err: Error) { |
| describe('wait for port', () => { | ||
| const servers: net.Server[] = []; | ||
| after(() => { | ||
| servers.forEach(server => server.close()); |
There was a problem hiding this comment.
Consider using a for...of loop for better readability and performance.
- servers.forEach(server => server.close());
+ for (const server of servers) {
+ server.close();
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| servers.forEach(server => server.close()); | |
| for (const server of servers) { | |
| server.close(); | |
| } |
| @@ -10,7 +10,7 @@ | |||
| "detect": "./bin/detect-port.js", | |||
| "detect-port": "./bin/detect-port.js" | |||
| }, | |||
| "main": "index.js", | |||
| "main": "./dist/commonjs/index.js", | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The output indicates that the build scripts in package.json do not explicitly mention generating the ./dist/commonjs/index.js file. This suggests that the build process might not be correctly configured to produce the expected output file.
- The
prepublishOnlyscript runstsc,tshy, andtshy-after, but none of these commands explicitly mention generating the./dist/commonjs/index.jsfile. - The
tscscript compiles./bin/detect-port.ts, which does not align with the expected output path.
To resolve this, you should ensure that the build process correctly generates the ./dist/commonjs/index.js file. This might involve updating the TypeScript configuration or the build scripts.
Analysis chain
Ensure the "main" entry points to the correct compiled output.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the main entry points to the correct compiled output.
# Test: Check if the main entry exists.
test -f ./dist/commonjs/index.js
Length of output: 34
Script:
#!/bin/bash
# Description: Verify if the build scripts in package.json are configured to generate the expected output file.
# Extract the build scripts from package.json
jq '.scripts' package.json
# Check if the expected output file is mentioned in the build configurations
grep -r './dist/commonjs/index.js' .
Length of output: 539
|
|
||
| it('should output available port randomly', { only: true }, async () => { | ||
| const res = await execaNodeWithLoader(binFile); | ||
| const port = parseInt(stripAnsi(res.stdout).trim(), 10); |
There was a problem hiding this comment.
Use Number.parseInt instead of parseInt to avoid using the global function.
- const port = parseInt(stripAnsi(res.stdout).trim(), 10);
+ const port = Number.parseInt(stripAnsi(res.stdout).trim(), 10);Also applies to: 50-50
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const port = parseInt(stripAnsi(res.stdout).trim(), 10); | |
| const port = Number.parseInt(stripAnsi(res.stdout).trim(), 10); |
|
|
||
| it('should output available port from the given port', async () => { | ||
| const givenPort = 9000; | ||
| const res = await execaNodeWithLoader(binFile, [ givenPort + '' ]); |
There was a problem hiding this comment.
Prefer template literals over string concatenation for better readability and potential performance benefits.
- const res = await execaNodeWithLoader(binFile, [ givenPort + '' ]);
+ const res = await execaNodeWithLoader(binFile, [`$givenPort`]);Committable suggestion was skipped due low confidence.
| @@ -50,7 +61,7 @@ function tryListen(port, maxPort, hostname, callback) { | |||
| if (hostname) { | |||
| listen(port, hostname, (err, realPort) => { | |||
| if (err) { | |||
| if (err.code === 'EADDRNOTAVAIL') { | |||
| if ((err as any).code === 'EADDRNOTAVAIL') { | |||
There was a problem hiding this comment.
Specify a more precise type than any for error handling.
- if ((err as any).code === 'EADDRNOTAVAIL') {
+ if ((err as NodeJS.ErrnoException).code === 'EADDRNOTAVAIL') {Also applies to: 94-94, 119-119
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if ((err as any).code === 'EADDRNOTAVAIL') { | |
| if ((err as NodeJS.ErrnoException).code === 'EADDRNOTAVAIL') { |
| export default function detectPort(port?: number | string | PortConfig | DetectPortCallback, callback?: DetectPortCallback) { | ||
| let hostname: string | undefined = ''; | ||
|
|
||
| if (port && typeof port === 'object') { |
There was a problem hiding this comment.
Avoid reassigning function parameters as it can lead to unexpected behavior and makes the code harder to understand.
- port = parseInt(port as unknown as string) || 0;
+ const parsedPort = Number.parseInt(port as unknown as string) || 0;Also applies to: 28-28, 32-32, 23-23, 27-27, 32-32, 51-51, 54-54, 55-55, 128-128
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (port && typeof port === 'object') { | |
| if (port && typeof port === 'object') { | |
| const parsedPort = Number.parseInt(port as unknown as string) || 0; |
BREAKING CHANGE: Drop Node.js < 16 support 1. 使用 ts 重构 2. 使用 tshy 支持 esm 和 cjs 3. test 使用 test-runner (这里需要 node v18 版本) merge from #51 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new `waitPort` function to asynchronously wait for a specified port to become available. - Added a new ESLint configuration to enforce TypeScript linting rules. - **Bug Fixes** - Reverted a feature in the `detect-port` package due to issues raised. - **Documentation** - Updated `README.md` for improved clarity and updated badge links. - Modified `CONTRIBUTING.md` to reflect changes in testing commands. - **Chores** - Introduced a new TypeScript configuration file (`tsconfig.json`). - Updated `package.json` to reflect changes in dependencies and project structure. - **Tests** - Added comprehensive tests for the new `waitPort` and updated tests for the CLI and `detectPort` function. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: hanquliu <admin@hanquliu-Mini.local>
[skip ci] ## [2.0.0](v1.6.1...v2.0.0) (2024-12-08) ### ⚠ BREAKING CHANGES * Drop Node.js < 16 support 1. 使用 ts 重构 2. 使用 tshy 支持 esm 和 cjs 3. test 使用 test-runner (这里需要 node v18 版本) merge from #51 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new `waitPort` function to asynchronously wait for a specified port to become available. - Added a new ESLint configuration to enforce TypeScript linting rules. - **Bug Fixes** - Reverted a feature in the `detect-port` package due to issues raised. - **Documentation** - Updated `README.md` for improved clarity and updated badge links. - Modified `CONTRIBUTING.md` to reflect changes in testing commands. - **Chores** - Introduced a new TypeScript configuration file (`tsconfig.json`). - Updated `package.json` to reflect changes in dependencies and project structure. - **Tests** - Added comprehensive tests for the new `waitPort` and updated tests for the CLI and `detectPort` function. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * refactor with typescript to support esm and cjs both ([#56](#56)) ([b5d32d2](b5d32d2))
BREAKING CHANGE: Drop Node.js < 16 support
Summary by CodeRabbit
New Features
waitPortfunction for asynchronously waiting for port availability.Updates
detectPortfunction with improved parameter handling and versatility.Bug Fixes
Chores
.gitignoreto exclude.tshy*files anddist/directory.package.json.Refactor