feat: support promise with timeout #64
Conversation
WalkthroughThe pull request introduces enhancements to the project's CI/CD workflow, package configuration, and utility functions. A new GitHub Actions workflow for package publishing is added, Node.js version support is expanded to version 23, and a comprehensive timeout utility is implemented. The changes include updating the README documentation, adding a new timeout module with error handling, and introducing corresponding unit tests to validate the new functionality. Changes
Sequence DiagramsequenceDiagram
participant Client
participant runWithTimeout
participant Promise
participant TimeoutPromise
Client->>runWithTimeout: Call with promise and timeout
runWithTimeout->>Promise: Start original promise
runWithTimeout->>TimeoutPromise: Create timeout promise
alt Promise resolves first
Promise-->>runWithTimeout: Return result
runWithTimeout-->>Client: Return result
else Timeout occurs first
TimeoutPromise-->>runWithTimeout: Throw TimeoutError
runWithTimeout-->>Client: Throw TimeoutError
end
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 dependencies detected. Learn more about Socket for GitHub ↗︎
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/timeout.ts (3)
1-10: LGTM! Consider adding readonly modifier.The TimeoutError implementation follows best practices for custom errors. Consider making the timeout property readonly since it shouldn't change after initialization.
- timeout: number; + readonly timeout: number;
12-29: Add validation for timeout parameter.The implementation is solid and handles cleanup properly. However, consider adding validation for the timeout parameter to prevent negative values.
export async function promiseTimeout<T>( promiseArg: Promise<T>, timeout: number, ): Promise<T> { + if (timeout <= 0) { + throw new Error('Timeout must be a positive number'); + } let timer: NodeJS.Timeout;
31-36: Add JSDoc documentation.Consider adding JSDoc documentation to explain the function's purpose, parameters, and potential errors.
+/** + * Executes an async function with a timeout limit + * @param scope - The async function to execute + * @param timeout - Maximum time in milliseconds to wait for execution + * @throws {TimeoutError} When execution time exceeds the timeout + * @returns Promise resolving to the scope function's result + */ export async function runWithTimeout<T>(test/timeout.test.ts (3)
1-3: Remove redundant imports.The specific imports from '../src/index.js' make the utility import redundant.
import { strict as assert } from 'node:assert'; -import * as utility from '../src/index.js'; import { runWithTimeout, TimeoutError, promiseTimeout } from '../src/index.js';
39-46: Fix duplicate test description.This test description is identical to the previous one but tests a different scenario.
- it('should timeout', async () => { + it('should resolve within timeout', async () => {
11-61: Improve test maintainability.Consider the following improvements:
- Extract magic numbers to named constants
- Remove commented console.error statements
- Add test cases for invalid inputs (negative timeout)
describe('test/timeout.test.ts', () => { + const TEST_TIMEOUTS = { + SHORT: 10, + MEDIUM: 20, + LONG: 100, + VERY_LONG: 1000 + }; + describe('runWithTimeout()', () => { it('should timeout', async () => { await assert.rejects(async () => { await runWithTimeout(async () => { - await sleep(20); - }, 10); + await sleep(TEST_TIMEOUTS.MEDIUM); + }, TEST_TIMEOUTS.SHORT); }, (err: unknown) => { assert(err instanceof TimeoutError); - assert.equal(err.timeout, 10); - assert.equal(err.message, 'Timed out after 10ms'); - // console.error(err); + assert.equal(err.timeout, TEST_TIMEOUTS.SHORT); + assert.equal(err.message, `Timed out after ${TEST_TIMEOUTS.SHORT}ms`); return true; });README.md (1)
123-123: Consider improving warning message formattingThe warning message about
getIP()removal could be more prominent with better markdown formatting.-___[WARNNING] `getIP()` remove, PLEASE use `https://github.com/node-modules/address` module instead.___ +> ⚠️ **WARNING**: `getIP()` has been removed. Please use [`address`](https://github.com/node-modules/address) module instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/nodejs.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks)README.md(5 hunks)package.json(2 hunks)src/index.ts(1 hunks)src/timeout.ts(1 hunks)test/timeout.test.ts(1 hunks)
🔇 Additional comments (6)
src/index.ts (1)
11-11: LGTM! Verify the timeout implementation.
The export statement follows the established pattern and correctly uses the .js extension.
Let's verify the timeout implementation:
✅ Verification successful
Let me check the implementation details of the timeout functionality.
Implementation verified and looks solid ✅
The timeout implementation is well-structured with:
- A proper
TimeoutErrorclass with stack trace capture - A robust
promiseTimeoutfunction usingPromise.racewith cleanup - A convenient
runWithTimeoutwrapper - Comprehensive test coverage for both success and timeout scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the timeout.js implementation and its usage
# Look for the timeout.js implementation
fd timeout.js
# Check for any existing timeout-related tests
fd timeout.test
# Look for usage of timeout functions in the codebase
rg -t ts "promiseTimeout|runWithTimeout|TimeoutError"
Length of output: 1097
Script:
#!/bin/bash
# Get the implementation details of timeout.ts
cat src/timeout.ts
# Get the test implementation to understand the usage patterns
cat test/timeout.test.ts
Length of output: 2709
.github/workflows/nodejs.yml (1)
15-17: Verify Node.js 23.x compatibility.
While adding Node.js 23 support is forward-looking, it's important to ensure compatibility with the timeout implementation and other features.
Let's check for any version-specific code or compatibility issues:
The CODECOV_TOKEN secret addition is good for maintaining code coverage reporting.
✅ Verification successful
Node.js 23.x compatibility verified
The codebase is compatible with Node.js 23.x based on the following findings:
- Package.json specifies engine requirement as
">= 16.0.0", which includes Node.js 23 - The timeout implementation uses standard Node.js APIs (Promise, setTimeout, clearTimeout) that are stable across versions
- Async/await and Promise usage follows standard patterns that are well-supported since Node.js 16
- No version-specific APIs or syntax are used in the codebase
- Dependencies (
escape-htmlandunescape) are basic string manipulation libraries without version constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for version-specific code and package.json engine constraints
# Check package.json for engine constraints
rg -t json '"engines":'
# Look for version-specific code or conditions
rg -t ts "process.version"
Length of output: 80
Script:
#!/bin/bash
# Let's check the actual engine constraints in package.json
cat package.json | jq .engines
# Check for any Node.js version-specific dependencies
cat package.json | jq .dependencies
# Look for any timeout or async-related implementations that might be version-sensitive
rg -g '!node_modules' -g '!dist' -l 'setTimeout|setInterval|promisify|async|await'
Length of output: 766
Script:
#!/bin/bash
# Let's examine the timeout implementation for any version-specific code
cat src/timeout.ts
# Check test cases to see if they're version-dependent
cat test/timeout.test.ts
# Look for any version-specific syntax in async implementations
rg -g '!node_modules' -g '!dist' -A 5 'async|await' src/
Length of output: 4209
package.json (1)
10-10: Verify semver bump and new dependency.
The changes look good. The version bump to 2.2.0 is appropriate for a new feature, and the @arethetypeswrong/cli addition enhances type checking.
Also applies to: 19-19
✅ Verification successful
Version bump and dependency changes are correctly implemented
The package uses the latest version of @arethetypeswrong/cli (^0.17.1) and the version bump to 2.2.0 is properly reflected in package.json.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the latest version of @arethetypeswrong/cli
curl -s https://registry.npmjs.org/@arethetypeswrong/cli/latest | jq '.version'
Length of output: 88
Script:
#!/bin/bash
# Check the version specified in package.json
cat package.json | jq '.devDependencies["@arethetypeswrong/cli"]'
# Check the current version and changes
cat package.json | jq '.version'
git show HEAD:package.json | jq '.version'
Length of output: 169
README.md (3)
7-7: LGTM: Node.js version badge addition
The badge addition helps users quickly identify Node.js version compatibility.
38-118: LGTM: Updated examples are clear and consistent
The examples effectively demonstrate the usage of each utility function with the updated import style.
33-33: Verify impact of import alias change
The change from utility to utils appears to be for consistency, but this could potentially be a breaking change if it affects the package's public API.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
123-123: Improve warning message formattingConsider using a more prominent and consistent warning format:
-___[WARNNING] `getIP()` remove, PLEASE use `https://github.com/node-modules/address` module instead.___ +> ⚠️ **WARNING**: The `getIP()` function has been removed. Please use the [`address`](https://github.com/node-modules/address) module instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(5 hunks)
🔇 Additional comments (3)
README.md (3)
7-7: LGTM: Node.js version badge addition
The badge addition follows the established pattern and provides valuable version support information.
33-33: LGTM: Consistent import statement updates
The standardization of import statements from utility to utils is applied consistently across all examples.
Also applies to: 39-39, 58-58, 77-77, 86-86, 96-96
197-208: Documentation needs more details for timeout functionality
The timeout documentation still needs improvement as previously noted.
Previous review suggestions remain valid:
- Explain the timeout parameter (units, constraints)
- Show error handling examples
- Describe return values
- Provide complete examples including timeout scenarios
[skip ci] ## [2.3.0](v2.2.0...v2.3.0) (2024-12-18) ### Features * support promise with timeout ([#64](#64)) ([bc3e168](bc3e168))
Summary by CodeRabbit
New Features
TimeoutErrorclass and timeout handling functions (promiseTimeout,runWithTimeout) for better asynchronous control.runWithTimeout.Bug Fixes
Documentation
Chores