feat: refactor with typescript to support esm and cjs both #56
Conversation
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces a new ESLint configuration file, updates the changelog, modifies the contribution guidelines, and restructures the README file. It also includes significant changes to 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected] |
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range and nitpick comments (2)
bin/detect-port.ts (1)
Line range hint
49-49: ReplaceisNaNwithNumber.isNaNfor more accurate checks that avoid type coercion.- } else if (isNaN(port)) { + } else if (Number.isNaN(port)) {src/detect-port.ts (1)
Line range hint
19-47: Refactor to avoid reassigning function parameters.Reassigning function parameters can lead to confusing bugs and harder-to-maintain code. Consider using local variables instead.
- export default function detectPort(port?: number | string | PortConfig | DetectPortCallback, callback?: DetectPortCallback) { + export default function detectPort(inputPort?: number | string | PortConfig | DetectPortCallback, inputCallback?: DetectPortCallback) { + let port = inputPort; + let callback = inputCallback;
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- .eslintrc (1 hunks)
- .github/workflows/nodejs.yml (1 hunks)
- .github/workflows/release.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 (4)
- .eslintrc
- .github/workflows/release.yml
- .gitignore
- tsconfig.json
Additional Context Used
Biome (29)
bin/detect-port.ts (2)
31-31: Use Number.parseInt instead of the equivalent global.
49-49: 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)
51-51: Template literals are preferred over string concatenation.
45-45: Use Number.parseInt instead of the equivalent global.
52-52: 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.
GitHub Check Runs (3)
Node.js / Test (ubuntu-latest, 22) failure (2)
bin/detect-port.ts: [failure] 8-8:
The 'import.meta' meta-property is only allowed when the '--module' option is 'es2020', 'es2022', 'esnext', 'system', 'node16', or 'nodenext'.Node.js / Test (ubuntu-latest, 20) failure (2)
bin/detect-port.ts: [failure] 8-8:
The 'import.meta' meta-property is only allowed when the '--module' option is 'es2020', 'es2022', 'esnext', 'system', 'node16', or 'nodenext'.Node.js / Test (ubuntu-latest, 18) failure (2)
bin/detect-port.ts: [failure] 8-8:
The 'import.meta' meta-property is only allowed when the '--module' option is 'es2020', 'es2022', 'esnext', 'system', 'node16', or 'nodenext'.
Additional comments not posted (5)
src/index.ts (2)
1-2: The imports are correctly updated to use ES module syntax.
4-6: The export statements are correctly updated to support both default and named exports, aligning with the PR's objectives to support ESM and CJS..github/workflows/nodejs.yml (1)
12-15: The workflow file has been updated to use the latest versions of Node.js, ensuring compatibility with the new features and dropping support for older versions as per the PR description.package.json (1)
13-13: The updates to thepackage.jsonalign with the PR's objectives, including the update of dependencies and scripts to support TypeScript and the new module structure.Also applies to: 24-39, 42-74
bin/detect-port.ts (1)
8-8: Ensure the TypeScript configuration allows the use ofimport.meta. This might require setting themoduleoption tonodenextor similar intsconfig.json.Verification successful
The TypeScript configuration in
tsconfig.jsonhas themoduleoption set tonodenext, which supports the use ofimport.meta.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the TypeScript configuration for module option. # Test: Search for the TypeScript configuration. Expect: Module option set to support 'import.meta'. cat tsconfig.json | jq '.compilerOptions.module'Length of output: 59
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
src/detect-port.ts (1)
94-94: Fix typographical error in the comment.In the comment, correct the word "unkonwn" to "unknown" for proper spelling.
src/wait-port.ts (1)
6-6: Use built-insetTimeoutfromnode:timers/promisesinstead of customsleepfunction.Node.js 16+ provides a Promise-based
setTimeoutinnode:timers/promises, which simplifies the code and removes the need for a customsleepfunction.Apply this diff to refactor:
- const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); + import { setTimeout as sleep } from 'node:timers/promises';test/wait-port.test.ts (2)
13-13: Improve test case description for clarity.The test description
'should be work'is grammatically incorrect. Consider rephrasing it to'should work'or'should wait for the port successfully'.Apply this diff to update the test description:
- it('should be work', async () => { + it('should wait for the port successfully', async () => {
25-25: Improve test case description for clarity.The test description
'should be work when retries exceeded'is unclear. Rephrase it to'should throw an error when retries are exceeded'for better understanding.Apply this diff to update the test description:
- it('should be work when retries exceeded', async () => { + it('should throw an error when retries are exceeded', async () => {bin/detect-port.cjs (2)
Line range hint
36-44: Consider adding error handling for the callbackThe error handling in the callback could be improved to ensure proper process exit codes on errors.
detectPort(random, (err, port) => { if (isVerbose) { if (err) { console.log(`get available port failed with ${err}`); + process.exit(1); } console.log(`get available port ${port} randomly`); } else { console.log(port || random); } });
Line range hint
65-80: Consider modernizing the callback patternSince this PR requires Node.js 16+, consider using async/await pattern for better readability and error handling.
- detectPort(port, (err, _port) => { + (async () => { + try { + const _port = await detectPort(port); if (isVerbose) { - if (err) { - console.log(`get available port failed with ${err}`); - } if (port !== _port) { console.log(`port ${port} was occupied`); } console.log(`get available port ${_port}`); } else { console.log(_port || port); } - }); + } catch (err) { + if (isVerbose) { + console.log(`get available port failed with ${err}`); + } + process.exit(1); + } + })();test/cli.test.ts (1)
34-38: Uncomment and fix the random port testThe commented test for random port output should either be removed or uncommented and fixed. If it's meant to be skipped temporarily, add a TODO comment explaining why.
README.md (1)
Line range hint
41-60: Add ESM import example alongside CommonJSSince the package now supports both ESM and CJS, the documentation should show both import styles.
Add ESM example:
```javascript const detect = require('detect-port'); /** * use as a promise */ +``` + +```javascript +// ESM +import detect from 'detect-port'; +```CHANGELOG.md (1)
75-75: Fix hyphenation in changelog entryThe term "double check" should be hyphenated as "double-check" for grammatical correctness.
- * fix: should double check 0.0.0.0 and localhost (#20) + * fix: should double-check 0.0.0.0 and localhost (#20)🧰 Tools
🪛 LanguageTool
[grammar] ~75-~75: The verb “double-check” is spelled with a hyphen.
Context: ...-11 ================== * fix: should double check 0.0.0.0 and localhost (#20) * docs: i...(DOUBLE_HYPHEN)
🪛 Markdownlint (0.35.0)
75-75: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
logo.pngis excluded by!**/*.png
📒 Files selected for processing (14)
.eslintrc(1 hunks)CHANGELOG.md(1 hunks)CONTRIBUTING.md(1 hunks)HISTORY.md(0 hunks)README.md(4 hunks)bin/detect-port.cjs(3 hunks)package.json(1 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 with no reviewable changes (1)
- HISTORY.md
✅ Files skipped from review due to trivial changes (3)
- CONTRIBUTING.md
- src/index.ts
- .eslintrc
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 890 characters long)
Context: ...t [![NPM version][npm-image]][npm-url] 
[![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][sny...
(EN_EXCESSIVE_EXCLAMATION)
CHANGELOG.md
[grammar] ~75-~75: The verb “double-check” is spelled with a hyphen.
Context: ...-11 ================== * fix: should double check 0.0.0.0 and localhost (#20) * docs: i...
(DOUBLE_HYPHEN)
🪛 eslint
bin/detect-port.cjs
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 4-4: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 5-5: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🪛 Markdownlint (0.35.0)
CHANGELOG.md
20-20: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
26-26: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
45-45: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
55-55: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
67-67: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
72-72: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
24-24: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
30-30: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
33-33: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
36-36: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
37-37: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
38-38: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
39-39: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
40-40: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
41-41: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
42-42: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
43-43: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
49-49: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
52-52: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
53-53: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
59-59: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
60-60: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
63-63: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
64-64: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
65-65: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
70-70: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
75-75: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
76-76: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
80-80: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
84-84: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
88-88: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
89-89: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
93-93: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
97-97: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
101-101: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
105-105: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
109-109: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
23-23: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
29-29: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
32-32: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
35-35: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
48-48: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
51-51: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
58-58: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
62-62: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (12)
src/detect-port.ts (2)
32-32: Use Number.parseInt instead of parseInt for clarity.
Using Number.parseInt improves readability by explicitly indicating the method's association with the Number object.
64-64: Avoid casting errors to any; use a more specific error type.
Casting errors to any defeats TypeScript's type safety. Consider using NodeJS.ErrnoException for better type specificity.
Also applies to: 94-94, 119-119
tsconfig.json (1)
6-6: Ensure the target ES2022 is compatible with Node.js 16.
Since Node.js 16 supports up to ES2021 features, setting the target to ES2022 might introduce features not supported in Node.js 16. Consider adjusting the target to ES2021 or verify compatibility.
src/wait-port.ts (1)
21-23: Avoid using any; define a custom error type for better type safety.
Using any undermines TypeScript's type system. Define a custom error interface to include retries and count properties.
Apply this diff to define a custom error type:
+ interface RetryError extends Error {
+ retries: number;
+ count: number;
+ }
- const err = new Error('retries exceeded');
- (err as any).retries = retries;
- (err as any).count = count;
+ const err: RetryError = new Error('retries exceeded') as RetryError;
+ err.retries = retries;
+ err.count = count;test/wait-port.test.ts (2)
10-10: Use for...of loop instead of forEach for better readability.
Replacing the forEach with a for...of loop enhances readability and maintains consistent iteration patterns.
Apply this diff to update the loop:
- servers.forEach(server => server.close());
+ for (const server of servers) {
+ server.close();
+ }29-29: Avoid using any in the catch block; specify the error type.
Using any defeats type safety. Specify the error as Error to maintain strong typing.
Apply this diff to update the error type:
- } catch (err:any) {
+ } catch (err: Error) {package.json (1)
68-70: Ensure compatibility between type, main, and exports configurations.
Setting "type": "module" while your main points to CommonJS may cause issues. Verify that the module, main, and exports fields are correctly configured to support both ESM and CommonJS environments.
bin/detect-port.cjs (1)
3-5: ESLint warnings about require() can be safely ignored
The .cjs extension explicitly indicates this is a CommonJS module, so using require() is appropriate here. The ESLint warnings can be suppressed as this is an intentional CommonJS file.
🧰 Tools
🪛 eslint
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 4-4: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 5-5: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/cli.test.ts (1)
42-42: Use template literals for string conversion
Replace string concatenation with template literals for better readability.
- const res = await execaNode(binFile, [ givenPort + '' ]);
+ const res = await execaNode(binFile, [ `${givenPort}` ]);test/detect-port.test.ts (3)
1-8: LGTM! Modern import practices
The use of the node: prefix for built-in modules follows Node.js best practices and improves module resolution.
104-104: Use template literals for string interpolation
Replace string concatenation with template literals for better readability.
- it('work with listening next port 24001 because 24000 was listened to ' + ip(), async () => {
+ it(`work with listening next port 24001 because 24000 was listened to ${ip()}`, async () => {53-55: 🛠️ Refactor suggestion
Improve server cleanup reliability
The server cleanup can be improved in two ways:
- Use for...of loop as suggested in previous reviews
- Add proper error handling and wait for servers to close
Consider refactoring to:
- after(() => {
- servers.forEach(server => server.close());
- });
+ after(async () => {
+ await Promise.all(
+ servers.map(server =>
+ new Promise<void>((resolve, reject) => {
+ server.close(err => err ? reject(err) : resolve());
+ })
+ )
+ );
+ });
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
package.json (1)
28-28: Consider using a lower @types/node versionThe current @types/node version (^22.10.1) is unnecessarily high for Node.js 16+ support. Consider using a version that matches your minimum Node.js version requirement.
- "@types/node": "^22.10.1", + "@types/node": "^16.18.0",src/bin/detect-port.ts (2)
Line range hint
18-25: Consider using filter for array manipulationThe current implementation mutates the array and uses a loop with splice. Consider using filter for a more functional and efficient approach.
-const removeByValue = (arr: string[], val: string) => { - for (let i = 0; i < arr.length; i++) { - if (arr[i] === val) { - arr.splice(i, 1); - break; - } - } -}; +const removeByValue = (arr: string[], val: string): string[] => { + return arr.filter(item => item !== val); +};
Line range hint
35-77: Consider modernizing with async/awaitThe callback-based error handling could be modernized using async/await for better readability and error handling.
Example transformation:
try { const _port = await detectPort(port); if (isVerbose) { if (port !== _port) { console.log(`port ${port} was occupied`); } console.log(`get available port ${_port}`); } else { console.log(_port || port); } } catch (err) { if (isVerbose) { console.log(`get available port failed with ${err}`); } process.exit(1); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
package.json(1 hunks)src/bin/detect-port.ts(4 hunks)test/cli.test.ts(1 hunks)test/detect-port.test.ts(1 hunks)test/wait-port.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/wait-port.test.ts
- test/detect-port.test.ts
- test/cli.test.ts
🔇 Additional comments (3)
package.json (3)
58-69: Well-structured dual package exports configuration
The exports field is properly configured to support both ESM and CJS with their respective type definitions, following Node.js best practices for dual packages.
40-46: Well-structured TypeScript build and test scripts
The scripts are properly configured to handle TypeScript compilation, testing, and publishing workflows.
48-50: Node.js version requirement aligns with PR objectives
The engine field correctly reflects the breaking change of dropping support for Node.js versions below 16.
[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
merge from #51
Summary by CodeRabbit
New Features
waitPortfunction to asynchronously wait for a specified port to become available.Bug Fixes
detect-portpackage due to issues raised.Documentation
README.mdfor improved clarity and updated badge links.CONTRIBUTING.mdto reflect changes in testing commands.Chores
tsconfig.json).package.jsonto reflect changes in dependencies and project structure.Tests
waitPortand updated tests for the CLI anddetectPortfunction.