Introduce Promise Helpers #2102
Conversation
|
Warning Rate limit exceeded@ardatan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 34 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. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a new dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CookieStore
participant PromiseHelpers
Client->>CookieStore: get(init)
CookieStore->>CookieStore: call getAll(init)
CookieStore->>PromiseHelpers: fakePromise(cookies)
PromiseHelpers-->>CookieStore: return promise-wrapped cookies
CookieStore->>Client: return first cookie via .then()
sequenceDiagram
participant Client
participant ServerAdapter
participant RequestHandler
participant PromiseHelpers
Client->>ServerAdapter: Send Request
ServerAdapter->>PromiseHelpers: handleMaybePromise(requestHandler)
PromiseHelpers->>RequestHandler: Invoke request handler
RequestHandler-->>PromiseHelpers: Return response or error
PromiseHelpers-->>ServerAdapter: Processed result
ServerAdapter->>Client: Return final response
Possibly related PRs
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 (
|
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@whatwg-node/cookie-store |
0.2.3-alpha-20250225122059-b9fa745cd0710ceae93952a89912c00202daf274 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/disposablestack |
0.0.6-alpha-20250225122059-b9fa745cd0710ceae93952a89912c00202daf274 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/node-fetch |
0.7.12-alpha-20250225122059-b9fa745cd0710ceae93952a89912c00202daf274 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/promise-helpers |
0.0.1-alpha-20250225122059-b9fa745cd0710ceae93952a89912c00202daf274 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/server |
0.9.70-alpha-20250225122059-b9fa745cd0710ceae93952a89912c00202daf274 |
npm ↗︎ unpkg ↗︎ |
✅
|
✅
|
✅
|
✅
|
✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/promise-helpers/src/index.ts (1)
46-78: Avoid using property names that conflict with Promise methods.The implementation of fakePromise creates an object with a 'then' property, which could cause issues with linters and potentially lead to unexpected behavior.
- then(resolve: (value: T) => any) { + // Use Symbol.species or another approach to create a Promise-compatible interface + [Symbol.species](resolve: (value: T) => any) {Alternatively, consider using a proper Promise subclass or extending Promise directly if your target environments support it.
🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Do not add then to an object.
(lint/suspicious/noThenProperty)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.changeset/@whatwg-node_cookie-store-2102-dependencies.md(1 hunks).changeset/@whatwg-node_disposablestack-2102-dependencies.md(1 hunks).changeset/@whatwg-node_node-fetch-2102-dependencies.md(1 hunks).changeset/@whatwg-node_server-2102-dependencies.md(1 hunks).changeset/green-rocks-bow.md(1 hunks)packages/cookie-store/package.json(1 hunks)packages/cookie-store/src/CookieStore.ts(4 hunks)packages/disposablestack/package.json(1 hunks)packages/disposablestack/src/AsyncDisposableStack.ts(3 hunks)packages/disposablestack/src/utils.ts(0 hunks)packages/node-fetch/package.json(1 hunks)packages/node-fetch/src/utils.ts(1 hunks)packages/promise-helpers/package.json(1 hunks)packages/promise-helpers/src/index.ts(1 hunks)packages/server/package.json(1 hunks)packages/server/src/createServerAdapter.ts(4 hunks)packages/server/src/plugins/useErrorHandling.ts(2 hunks)packages/server/src/utils.ts(2 hunks)packages/server/src/uwebsockets.ts(2 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/disposablestack/src/utils.ts
✅ Files skipped from review due to trivial changes (6)
- .changeset/@whatwg-node_server-2102-dependencies.md
- .changeset/green-rocks-bow.md
- .changeset/@whatwg-node_node-fetch-2102-dependencies.md
- .changeset/@whatwg-node_disposablestack-2102-dependencies.md
- .changeset/@whatwg-node_cookie-store-2102-dependencies.md
- packages/promise-helpers/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 53-53: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (35)
packages/disposablestack/package.json (1)
37-39: Dependency Addition: Approved Inclusion
The new dependency"@whatwg-node/promise-helpers": "^0.0.0"is correctly introduced here. This aligns with the broader initiative to centralize promise utilities across multiple packages, ensuring a uniform approach.packages/server/package.json (1)
37-39: New Dependency for Promise Helpers
The addition of"@whatwg-node/promise-helpers": "^0.0.0"in the dependencies is consistent with the PR's objectives. It ensures that promise-related utilities (such ashandleMaybePromise) are standardized across packages.packages/node-fetch/package.json (1)
37-39: Standardizing Promise Utilities
The inclusion of"@whatwg-node/promise-helpers": "^0.0.0"in the dependencies is correctly applied. This change contributes to unifying how promise handling is implemented across the project.packages/cookie-store/package.json (1)
37-39: Unified Dependency Addition for Promise Helpers
Adding"@whatwg-node/promise-helpers": "^0.0.0"to the dependencies ensures that the promise utility functions (e.g.,fakePromise) are resolved uniformly. This is consistent with changes made in related packages and supports maintainability.tsconfig.json (1)
33-34: Path Mapping Addition for Promise Helpers
The new path mapping entry"@whatwg-node/promise-helpers": ["packages/promise-helpers/src/index.ts"]enables TypeScript to correctly resolve the new package. This addition integrates seamlessly with the project’s module resolution strategy.
packages/node-fetch/src/utils.ts (1)
28-28: Good refactoring to use the dedicated promise helpers package.Using the standardized
fakePromiseimplementation from@whatwg-node/promise-helpersimproves code maintainability by centralizing the Promise utility implementation in a dedicated package.packages/server/src/plugins/useErrorHandling.ts (2)
2-2: Good dependency adoption.Appropriate import of the new promise utility from the dedicated package.
58-64: Excellent refactoring of error handling logic.The refactoring to use
handleMaybePromisesimplifies the code by eliminating explicit promise checking and try/catch boilerplate. This approach provides a cleaner way to handle both synchronous and asynchronous responses with unified error handling.packages/server/src/createServerAdapter.ts (6)
3-3: Good dependency adoption.Appropriate import of the new promise utility from the dedicated package.
188-202: Clean refactoring of response hook execution.The refactoring to use
handleMaybePromisesimplifies promise handling for response hooks, making the code more maintainable.
206-209: Streamlined early response handling.Good use of
handleMaybePromiseto simplify the early response logic, eliminating complex conditional promise handling.
213-237: Improved request hook execution flow.The refactored code uses
handleMaybePromiseto simplify the request hook execution process, making the code more readable and maintainable while preserving functionality.
273-292: Streamlined Node request and response handling.Good refactoring to use nested
handleMaybePromisecalls, which clearly separates the request handling, response handling, and error handling logic.
327-345: Improved UWS response handling.The refactored code for μWebSockets (UWS) response handling uses
handleMaybePromiseconsistently with other parts of the codebase, improving maintainability and error handling.packages/server/src/uwebsockets.ts (2)
1-1: Good dependency adoption.Appropriate import of the
fakePromiseutility from the dedicated package.
273-273: Maintained export API compatibility.Good practice to maintain API compatibility by re-exporting the
fakePromisefunction, allowing existing code to continue working without changes.packages/server/src/utils.ts (3)
5-5: Good use of external module to consolidate promise utilities.Replacing in-house promise utility functions with a dedicated module helps improve codebase maintainability.
8-9: LGTM! Maintaining backward compatibility.Re-exporting the imported functions maintains the public API while benefiting from the consolidated implementation.
450-450: LGTM! Added export of iterateAsyncVoid.Properly exports the utility function from the new promise-helpers package.
packages/cookie-store/src/CookieStore.ts (5)
1-1: LGTM! Importing fakePromise utility.Importing the utility from a dedicated package improves maintainability.
26-34: LGTM! Improved promise chaining.Changed from using async/await to promise chaining with
.then(). This maintains the same functionality while aligning with the new codebase promise handling approach.
99-103: LGTM! Simplified promise handling.Using fakePromise to consistently return a promise object without the overhead of async/await syntax.
110-111: LGTM! Consistent promise wrapping for filtered results.Properly wraps the filtered cookie array with fakePromise for consistent behavior.
113-133: LGTM! Simplified promise chain in delete method.Removed the async keyword and directly returns the promise from set() method, which is cleaner and more efficient.
packages/disposablestack/src/AsyncDisposableStack.ts (5)
1-1: LGTM! Using handleMaybePromiseLike for better promise handling.Importing the specialized utility for handling promise-like objects.
9-9: Enhanced type flexibility with MaybePromiseLike.Updated type from MaybePromise to MaybePromiseLike, which provides better compatibility with various promise-like objects beyond standard Promises.
23-23: Consistent type update in adopt method parameter.Changed from MaybePromise to MaybePromiseLike to align with the callbacks array type.
30-30: Consistent type update in defer method parameter.Changed from MaybePromise to MaybePromiseLike to align with the callbacks array type.
49-60: Simplified error handling with handleMaybePromiseLike.Refactored the _iterateCallbacks method to use handleMaybePromiseLike, which significantly improves readability and maintainability by delegating complex promise handling logic to a specialized utility.
packages/promise-helpers/src/index.ts (6)
1-2: LGTM! Well-defined type aliases for promise handling.Clear definition of MaybePromise and MaybePromiseLike types that will enable consistent promise type handling across the codebase.
4-10: LGTM! Simple and effective promise type checking utilities.The isPromise and isPromiseLike functions provide a standardized way to check for promise-like behavior, with isPromise delegating to isPromiseLike for consistency.
12-32: LGTM! Robust promise-like handling utility.The handleMaybePromiseLike function elegantly handles both promise-like and non-promise values, with proper error handling and factory functions for success and error cases.
34-44: LGTM! Type-safe wrapper for handleMaybePromiseLike.handleMaybePromise ensures the return type is specifically MaybePromise rather than the more general MaybePromiseLike.
80-105: LGTM! Modern approach to deferred promises.Excellent implementation that uses Promise.withResolvers when available (modern browsers) and falls back to a manual implementation for older environments.
107-132: LGTM! Effective async iteration utility.The iterateAsyncVoid function provides a clean way to iterate over collections with async callbacks, including the ability to stop iteration early.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/promise-helpers/src/index.ts (1)
46-77: Address linting issue with the 'then' propertyThe
fakePromiseimplementation has a linting error about adding 'then' to an object. While functionally correct, consider addressing this to maintain code quality standards.Since the object is specifically designed to mimic a Promise, you could use a more explicit approach to satisfy the linter:
- return { + // Define a fake promise without triggering linting issues + const fakePromiseObj: Promise<T> = { then(resolve: (value: T) => any) { // ...existing implementation... }, // ...other methods... [Symbol.toStringTag]: 'Promise', }; + return fakePromiseObj;🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Do not add then to an object.
(lint/suspicious/noThenProperty)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/disposablestack/tests/disposablestack.spec.ts(2 hunks)packages/promise-helpers/src/index.ts(1 hunks)packages/server/src/createServerAdapter.ts(5 hunks)packages/server/src/plugins/useCors.ts(4 hunks)packages/server/src/plugins/useErrorHandling.ts(2 hunks)packages/server/src/types.ts(6 hunks)packages/server/src/utils.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Check: e2e / cloudflare-modules
packages/server/src/utils.ts
[failure] 480-480:
Cannot find name 'MaybePromise'.
packages/server/src/createServerAdapter.ts
[failure] 405-405:
Cannot find name 'MaybePromise'.
🪛 GitHub Check: type check
packages/server/src/utils.ts
[failure] 480-480:
Cannot find name 'MaybePromise'.
packages/server/src/createServerAdapter.ts
[failure] 405-405:
Cannot find name 'MaybePromise'.
🪛 GitHub Check: e2e / cloudflare-workers
packages/server/src/utils.ts
[failure] 480-480:
Cannot find name 'MaybePromise'.
packages/server/src/createServerAdapter.ts
[failure] 405-405:
Cannot find name 'MaybePromise'.
🪛 GitHub Check: e2e / azure-function
packages/server/src/utils.ts
[failure] 480-480:
Cannot find name 'MaybePromise'.
packages/server/src/createServerAdapter.ts
[failure] 405-405:
Cannot find name 'MaybePromise'.
🪛 GitHub Check: e2e / aws-lambda
packages/server/src/utils.ts
[failure] 480-480:
Cannot find name 'MaybePromise'.
packages/server/src/createServerAdapter.ts
[failure] 405-405:
Cannot find name 'MaybePromise'.
🪛 Biome (1.9.4)
packages/server/src/plugins/useErrorHandling.ts
[error] 47-47: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/promise-helpers/src/index.ts
[error] 53-53: Do not add then to an object.
(lint/suspicious/noThenProperty)
🪛 GitHub Actions: test
packages/server/src/createServerAdapter.ts
[error] 405-405: TypeScript error TS2304: Cannot find name 'MaybePromise'.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: alpha / snapshot
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: unit / node 18
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: lint
- GitHub Check: server (undici)
- GitHub Check: esm
- GitHub Check: server (ponyfill)
- GitHub Check: prettier
- GitHub Check: server (native)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (27)
packages/disposablestack/tests/disposablestack.spec.ts (2)
1-2: Enhanced type system with promise-helpers import.The new import of
MaybePromisefrom@whatwg-node/promise-helpersbrings a more standardized approach to handling types that could be either a direct value or a Promise.
15-15: Improved type signature using MaybePromise.Replacing
void | Promise<void>withMaybePromise<void>improves readability and consistency with the rest of the codebase while maintaining the same behavior.packages/server/src/utils.ts (3)
5-5: Standardizing promise utilities with shared dependency.Importing
createDeferredPromiseandisPromisefrom@whatwg-node/promise-helperscentralizes these utilities in a dedicated package, promoting code reuse and consistency.
8-8: Re-exporting promise utilities.Re-exporting functions maintains backward compatibility for existing consumers while utilizing the standardized implementation.
450-450: Simplified export by using imported implementation.Replacing a local implementation with an imported utility from a dedicated package reduces code duplication and maintenance burden.
packages/server/src/plugins/useErrorHandling.ts (2)
2-2: Importing promise utilities from central package.Importing from a dedicated package enhances consistency in promise handling across the codebase.
47-47:❓ Verification inconclusive
Simplified type signature with MaybePromise.
Using
MaybePromise<Response>improves the type definition by concisely capturing both synchronous and asynchronous return paths.
🌐 Web query:
Is void confusing inside a union type in TypeScript?💡 Result:
Yes, using
voidinside union types in TypeScript can lead to confusion and unintended behavior. Here's why:Key Issues with
voidin Unions
Type Compatibility Problems
voidrepresents the absence of a meaningful value from functions, but when used in unions:type ConfusingUnion = number | void;
voidcannot be assigned to other types likeundefinedornull[1][7]- Variables of this type cannot safely interact with values expecting concrete types
Misleading Developer Expectations
voidin parameter positions suggests "no value needed," but TypeScript allows passingundefinedforvoidparameters[5][7]- Union members become effectively unrepresentable at runtime since
voidonly exists in type spaceLinter Enforcement
Biome'snoConfusingVoidTyperule specifically flags these cases:type PossibleValues = number | void; // ✖ lint error [1]The suggested fix replaces
voidwithundefined, though this is marked "unsafe" due to compatibility differences[1]Practical Example of Confusion
function handleValue(value: string | void) { // ❌ Can't safely use string methods without type guards if (typeof value === 'string') { console.log(value.toUpperCase()); } }Here, the
voidcase forces excessive type checking for a parameter that should either be a string or explicitly optional[5][7].Recommended Alternatives
Use Case Instead of voidBetter Choice Optional values `T void` Function returns (): voidKeep as-is Generic contexts <T extends void>Use judiciously The TypeScript documentation notes that unions appear to have intersection-like behavior for property access, which compounds confusion when
voidis involved[5]. Best practice is to reservevoidstrictly for function return types and useundefinedfor optional value positions[1][4][7].Citations:
- 1: https://biomejs.dev/linter/rules/no-confusing-void-type/
- 2: https://www.hacklewayne.com/unions-are-untagged-and-should-be-discriminated-in-typescript-undecidable-collapse-and-anti-patterns
- 3: https://www.youtube.com/watch?v=5eDUCK327cY
- 4: https://www.codecademy.com/learn/learn-typescript/modules/learn-typescript-union-types/cheatsheet
- 5: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html
- 6: https://www.hacklewayne.com/typescript-fireworks-union-intersection-and-variance
- 7: #AvoidTheVoid Allow users to avoid the
voidtype. microsoft/TypeScript#42709- 8: https://www.typescriptlang.org/docs/handbook/2/functions.html
Potential clarity issue: Consider replacing
voidwithundefinedfor improved type clarityThe simplified signature using
MaybePromise<Response>concisely supports both synchronous and asynchronous returns. However, as confirmed by our research, includingvoidin the union (MaybePromise<Response> | void) can be confusing. It might be clearer to represent the absence of a return value explicitly—e.g., by usingMaybePromise<Response | undefined>—unless the current approach is a deliberate design choice. If you choose to keep it as is, please ensure the behavior is well-documented to avoid future misunderstandings.
- File:
packages/server/src/plugins/useErrorHandling.ts(Line 47)🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/server/src/plugins/useCors.ts (3)
1-1: Adopting centralized promise helpers.Importing
handleMaybePromiseandMaybePromisefrom the dedicated package aligns with the broader refactoring effort.
25-25: Type enhancement with MaybePromise.Updating the return type to
MaybePromise<CORSOptions>improves readability while capturing the same semantics as the previous type.
148-159: Consistent promise handling in onResponse method.Using
handleMaybePromisehere creates a uniform approach to handling promises throughout the codebase.The code is cleaner and follows the same pattern established in other parts of the codebase.
packages/server/src/createServerAdapter.ts (6)
188-202: Good refactoring of promise handling with handleMaybePromiseThis is a good refactoring that simplifies the code by using the
handleMaybePromiseutility function from the new promise-helpers package. The previous implementation likely had more verbose promise handling with explicit checks for promises.
206-209: Simplified promise handling with handleMaybePromiseGood use of the
handleMaybePromisefunction to simplify the control flow. This makes the code more readable and easier to maintain.
213-237: Improved request hooks implementationThe refactoring from direct promise handling to using
handleMaybePromisemakes the code more maintainable and consistent in how it deals with asynchronous operations.
273-292: Enhanced error handling with handleMaybePromiseThe nested
handleMaybePromisecalls provide a cleaner approach to handling errors at different stages of the request processing pipeline. This is more maintainable than separate try-catch blocks.
327-345: Consistent promise handling for UWS requestsThis change ensures that UWS request handling follows the same pattern as other request types in the application, improving consistency across the codebase.
404-405: Fix the MaybePromise type importThe return type now uses
MaybePromise, but the type is not imported, causing TypeScript errors.🧰 Tools
🪛 GitHub Check: e2e / cloudflare-modules
[failure] 405-405:
Cannot find name 'MaybePromise'.🪛 GitHub Check: type check
[failure] 405-405:
Cannot find name 'MaybePromise'.🪛 GitHub Check: e2e / cloudflare-workers
[failure] 405-405:
Cannot find name 'MaybePromise'.🪛 GitHub Check: e2e / azure-function
[failure] 405-405:
Cannot find name 'MaybePromise'.🪛 GitHub Check: e2e / aws-lambda
[failure] 405-405:
Cannot find name 'MaybePromise'.🪛 GitHub Actions: test
[error] 405-405: TypeScript error TS2304: Cannot find name 'MaybePromise'.
packages/promise-helpers/src/index.ts (6)
1-2: Good type definitions for flexible promise handlingThese type definitions provide a clean way to represent values that can be either promises or direct values, enhancing type safety while maintaining flexibility.
4-10: Clean implementation of promise type guardsThese utility functions provide a concise way to check if a value is a promise or promise-like, which is useful for conditional handling of synchronous and asynchronous values.
12-32: Flexible promise handling with appropriate error recoveryThe
handleMaybePromiseLikefunction provides a robust way to handle both synchronous and asynchronous values with proper error handling. The internal structure with_handleMaybePromiseLikehelps avoid code duplication while maintaining clear error boundaries.
34-44: Type-safe wrapper for promise handlingThis function provides a more specific type signature based on
handleMaybePromiseLike, ensuring that the return type is consistently typed asMaybePromise<TOutput>.
80-105: Modern and fallback implementation for deferred promisesGood approach to utilize
Promise.withResolverswhen available, with a clean fallback implementation. This provides optimal performance on modern JS engines while maintaining compatibility.
107-132: Efficient asynchronous iteration with early termination supportThe
iterateAsyncVoidfunction provides a well-designed approach to asynchronous iteration with support for early termination. It effectively leverageshandleMaybePromisefor consistent promise handling.packages/server/src/types.ts (5)
2-2: Good type imports for promise handlingImporting both
MaybePromiseandMaybePromiseLiketypes provides the flexibility needed across the type system.
7-9: Enhanced FetchEvent interface with flexible promise typesThe updated method signatures in the
FetchEventinterface provide better flexibility by allowing both synchronous and asynchronous values using theMaybePromiseandMaybePromiseLiketypes.
31-48: More flexible method signatures in ServerAdapterObjectThese changes to the return types of the methods in the
ServerAdapterObjectinterface provide better flexibility by allowing both synchronous and asynchronous returns withMaybePromise<Response>.
57-98: Consistent use of MaybePromise across handler methodsThe consistent use of
MaybePromise<Response>as the return type for all handler methods ensures a uniform interface throughout the server adapter.
128-128: Updated ServerAdapterRequestHandler for consistent promise handlingThis change ensures that the
ServerAdapterRequestHandlertype aligns with the method signatures in the interfaces, maintaining consistency throughout the type system.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/server/src/createServerAdapter.ts (2)
188-202: Simplified promise handling in handleResponse function.The refactoring uses
handleMaybePromiseto replace the previous manual promise detection and handling logic. This makes the code more readable and consistent by abstracting away the promise-checking boilerplate.
213-237: Streamlined request hooks iteration with handleMaybePromise.Refactored the request hooks iteration to use
handleMaybePromise, which simplifies the code by removing manual promise handling and provides a more consistent approach to promise flow control.packages/promise-helpers/src/index.ts (1)
53-88: Lint Warning forthenProperty
The customfakePromiseprovides a neat fallback for non-promise values by imitating the full API surface of aPromise. However, the static analysis tool flags adding athenproperty on an object. While this pattern is intentional for creating a “thenable,” some linters label it suspicious. If you need to silence the lint warning, consider either disabling that rule locally or returning a genuine newPromiseto avoid confusion:-export function fakePromise<T>(value: T): Promise<T> { - if (isPromise(value)) { - return value; - } - return { - then(resolve) { ... }, - ... - } +export function fakePromise<T>(value: T): Promise<T> { + return new Promise((resolve) => resolve(value)); }Whether you keep the existing approach or switch to a new
Promiseis a design choice. Just be aware of potential confusion in some environments.🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Do not add then to an object.
(lint/suspicious/noThenProperty)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
deno.json(1 hunks)packages/node-fetch/src/fetchCurl.ts(1 hunks)packages/node-fetch/src/utils.ts(1 hunks)packages/promise-helpers/src/index.ts(1 hunks)packages/server/src/createServerAdapter.ts(5 hunks)packages/server/src/utils.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/node-fetch/src/fetchCurl.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 60-60: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: esm
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: alpha / snapshot
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: server (undici)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: server (ponyfill)
- GitHub Check: unit / node 18
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: type check
- GitHub Check: server (native)
- GitHub Check: prettier
- GitHub Check: e2e / azure-function
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: e2e / aws-lambda
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (19)
deno.json (1)
11-12: Added the promise helpers package to imports.The import mapping for
@whatwg-node/promise-helpershas been correctly added, pointing to the package's entry point. This change enables the use of promise utilities throughout the project.packages/node-fetch/src/utils.ts (1)
28-28: Centralized promise utilities by importing from dedicated package.The local implementation of
fakePromisehas been replaced with an import from the dedicated@whatwg-node/promise-helperspackage. This change follows the DRY principle by centralizing utility functions in a specialized package rather than duplicating implementations across the codebase.packages/server/src/utils.ts (4)
5-5: Imported promise utilities from dedicated package.You've added the necessary imports from
@whatwg-node/promise-helpersincludingMaybePromise, which was previously identified as missing and causing build failures. Good job addressing this issue.
8-8: Re-exported promise utilities for backward compatibility.Re-exporting these functions maintains backward compatibility for other modules that may be importing these utilities from this file rather than directly from the new package.
450-450: Imported iterateAsyncVoid from dedicated package.Replacing the local implementation with an import from the dedicated package helps with maintaining a single source of truth for promise-related utilities.
480-480: Updated parameter type to use MaybePromise.Updating the parameter type from
Promise<Response> | ResponsetoMaybePromise<Response>provides better type safety and more clearly expresses the intent that this function can handle both promises and direct values.packages/server/src/createServerAdapter.ts (5)
3-3: Imported promise helper utilities.Added imports for both
handleMaybePromiseandMaybePromisefrom the dedicated promise helpers package, which will help simplify promise handling throughout this file.
205-210: Improved error handling in handleEarlyResponse.Using
handleMaybePromisefor handling both promise and non-promise values eliminates potential edge cases in promise handling and ensures consistent error propagation.
273-293: Enhanced error handling in requestListener.The nested
handleMaybePromisecalls create a clean chain of promise handling with proper error handling at each step. This ensures errors are appropriately caught and logged without interrupting the request flow.
327-346: Improved promise handling in handleUWS.Similar to the requestListener function, this refactoring ensures consistent promise handling and error management when dealing with UWS responses, making the code more robust.
405-405: Updated return type to use MaybePromise.The return type now accurately reflects that this function can return either a direct value or a promise, using the more expressive
MaybePromisetype from the new package.packages/promise-helpers/src/index.ts (8)
1-2: Well-structured Generic Types
The definitions ofMaybePromiseandMaybePromiseLikeare straightforward and neatly capture values that may or may not be wrapped in a promise. This pattern will help ensure flexible handling of both synchronous and asynchronous flows.
4-6: Utility Specialization
isPromiseis a suitable convenience function building onisPromiseLike. This maintains readability in call sites that only need to confirm a fullPromise.
8-12: Checks forthenPresence
Using(value as TPromiseLike)?.then != nullis a succinct way to detect promise-like objects. The approach seems consistent and minimal.
14-39: Flexible Promise-Like Handling
handleMaybePromiseLikecorrectly addresses both promise-like objects and immediate values, applying success or error transformations as needed. The layering of_handleMaybePromiseLikewithin a try-catch ensures synchronous exceptions are properly caught. This function appears robust for a variety of asynchronous patterns.
41-51: Convenient Wrapper
handleMaybePromiseis a more specialized variant ofhandleMaybePromiseLike, defaulting to nativePromise. This keeps the code DRY and simplifies usage in typical promise-based workflows.
90-94: Clear Deferred Interface
DeferredPromiseneatly exposes the internal resolve and reject methods. This is a familiar pattern for capturing references to a promise’s resolution functions without special wrappers.
96-115: Conditionally UsingPromise.withResolvers
createDeferredPromisechecks forPromise.withResolvers, an experimental feature. Ensure your environment or build setup supports this. Otherwise, the fallback logic with a regularnew Promiseis correctly implemented.Would you like to confirm compatibility in older Node versions or browsers? A simple approach is searching for usage references to see if it’s guarded or polyfilled.
117-142: Asynchronous Iteration
iterateAsyncVoideffectively processes iterable items in sequence, short-circuiting whenstopEarlyis invoked. This approach is straightforward and easy to follow, relying onhandleMaybePromisefor controlling asynchronous flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/promise-helpers/src/index.ts (3)
8-12: Suggest verifying thatthenis indeed callable.Currently,
isPromiseLikechecks whethervalue.then != null. For stricter type checking, consider verifying thatthenis a function:-return (value as TPromiseLike)?.then != null; +return typeof (value as TPromiseLike)?.then === 'function';This helps ensure the object truly behaves like a promise.
53-88: Heed lint warning: Avoid assigning.thento plain objects.The static analysis tool (lint/suspicious/noThenProperty) flags this as suspicious. While this custom “fake” promise may be intentional, it’s an uncommon pattern that can cause confusion or break third-party libraries expecting standard promise constructors. If possible, refactor to use real Promises or a minimal polyfill:
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Do not add then to an object.
(lint/suspicious/noThenProperty)
144-166: Again, address lint warning about.thenproperty.Similar to
fakePromise,fakeRejectPromiseattaches.thenand others to a plain object. If this is strictly necessary, consider an explanatory comment or refactor to avoid potential pitfalls and confusion with native Promise semantics.🧰 Tools
🪛 Biome (1.9.4)
[error] 149-149: Do not add then to an object.
(lint/suspicious/noThenProperty)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 60-60: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 149-149: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: unit / node 23
- GitHub Check: unit / deno
- GitHub Check: unit / node 22
- GitHub Check: esm
- GitHub Check: unit / bun
- GitHub Check: alpha / snapshot
- GitHub Check: unit / node 20
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: unit / node 18
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: server (undici)
- GitHub Check: lint
- GitHub Check: e2e / azure-function
- GitHub Check: server (ponyfill)
- GitHub Check: prettier
- GitHub Check: e2e / aws-lambda
- GitHub Check: server (native)
- GitHub Check: type check
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/promise-helpers/src/index.ts (2)
14-39: Clarify synchronous vs. asynchronous error flow.The synchronous
try/catchinhandleMaybePromiseLikewon't handle errors thrown asynchronously within a Promise chain. If an asynchronous error occurs without an error callback, it may remain unhandled. Consider clearly documenting or testing this behavior to avoid confusion in callers.
96-115: Check availability ofPromise.withResolvers.
Promise.withResolversis not yet a standard API in all JavaScript runtimes. If this code runs in environments that do not support it, fallback or polyfill logic will be essential. Verify that build and runtime targets are compatible, and consider documenting this implementation detail.
Create a helper package that collects all helpers and hacks related to
PromiseSummary by CodeRabbit
New Features
Refactor