feat(promise-helpers): add finally callback to handleMaybePromise
#2152
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request enhances the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 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 (
|
✅
|
✅
|
✅
|
✅
|
✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/promise-helpers/tests/handleMaybePromise.spec.ts (2)
26-33: Incorrect test descriptionThe test description mentions "fake promise" but the test is actually validating error handling for a rejected promise.
- it('should call finally and allow chaining with a fake promise', async () => { + it('should call finally and allow chaining with a rejected promise', async () => {
6-36: Additional test coverage neededThe tests only cover scenarios where all callbacks are provided. Consider adding tests for cases when the
finallyFactoryis not provided or whenonErroris not provided to ensure the error propagation works as expected.Here's a suggestion for additional test cases:
it('should work correctly when finallyFactory is not provided', async () => { expect( await handleMaybePromise(() => Promise.resolve('test'), onSuccess, onError) ).toBe('test'); expect(onSuccess).toHaveBeenCalledTimes(1); expect(onError).not.toHaveBeenCalled(); }); it('should throw when onError is not provided and promise rejects', async () => { await expect( handleMaybePromise(() => Promise.reject('error'), onSuccess) ).rejects.toBe('error'); expect(onSuccess).not.toHaveBeenCalled(); }); it('should throw and still call finally when onError is not provided', async () => { await expect( handleMaybePromise(() => Promise.reject('error'), onSuccess, undefined, onFinally) ).rejects.toBe('error'); expect(onSuccess).not.toHaveBeenCalled(); expect(onFinally).toHaveBeenCalledTimes(1); });packages/promise-helpers/src/index.ts (1)
42-60: Consider simplifying the nested promise chainsThe implementation works correctly but is complex with multiple nested calls to
handleMaybePromise. This could potentially be simplified for better readability.Consider whether Promise.prototype.finally could be directly used when available, with a fallback for environments where it's not supported or for non-promise returns. This might make the code more straightforward, especially since the finallyFactory is guaranteed to be called regardless of success or failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/promise-helpers/src/index.ts(2 hunks)packages/promise-helpers/tests/handleMaybePromise.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: unit / bun
- GitHub Check: server (undici)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (ponyfill)
- GitHub Check: lint
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: server (native)
🔇 Additional comments (5)
packages/promise-helpers/tests/handleMaybePromise.spec.ts (1)
17-24: LGTM! Test validates success path properlyThis test correctly verifies that both the success callback and finally callback are called when a promise resolves successfully, and that the error callback is never invoked.
packages/promise-helpers/src/index.ts (4)
14-14: LGTM! Consistent type definitions for finallyFactoryThe
finallyFactoryparameter is consistently defined across all three function overloads, providing type safety for this new feature.Also applies to: 20-20, 26-26
42-63: The implementation correctly handles the new finallyFactory parameterThe conditional logic to incorporate the
finallyFactorycallback is cleanly implemented, ensuring that finallyFactory is called regardless of whether the promise resolves or rejects.
65-69: LGTM! Error handling improved with explicit throwThe error handling now properly throws the error when no
outputErrorFactoryis provided, which improves error propagation.
41-70:✅ Verification successful
Verify backward compatibility
This change adds a new optional parameter which should maintain backward compatibility, but it's worth verifying that existing usage patterns still work as expected.
🏁 Script executed:
#!/bin/bash # Search for existing usage of handleMaybePromise in the codebase rg --type ts "handleMaybePromise\(" -A 3 -B 1Length of output: 10192
Backward compatibility verified—no breakage observed.
After reviewing the test cases in packages/promise-helpers/tests/handleMaybePromise.spec.ts and various usage sites (e.g., in packages/server), it’s clear that the integration of the new optional parameter (finallyFactory) does not disrupt existing behavior. Both success and error handling paths remain consistent with previous patterns, confirming that existing usage patterns continue to work as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/promise-helpers/src/index.ts (1)
314-338: Well-implementedpromiseLikeFinallyhelper function.This helper function correctly handles both promises with native
finallysupport and those without it. The implementation properly ensures that:
- For successful promises, the finally callback executes and the original result is preserved
- For failed promises, the finally callback executes and the original error is rethrown
Consider slightly simplifying the error handling path to reduce duplication:
- err => { - const finallyRes = onFinally(); - if (isPromise(finallyRes)) { - return finallyRes.then(() => { - throw err; - }); - } else { - throw err; - } - }, + err => { + const finallyRes = onFinally(); + const throwError = () => { throw err; }; + return isPromise(finallyRes) ? finallyRes.then(throwError) : throwError(); + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/late-flies-bet.md(1 hunks)packages/promise-helpers/src/index.ts(3 hunks)packages/promise-helpers/tests/handleMaybePromise.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/promise-helpers/tests/handleMaybePromise.spec.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/late-flies-bet.md
[grammar] ~5-~5: You used an adverb (‘finally’) instead of an adjective, or a noun (‘callback’) instead of another adjective.
Context: ...se-helpers': minor --- Allow to pass a finally callback to handleMaybePromise
(A_RB_NN)
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 170-170: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: unit / bun
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (6)
packages/promise-helpers/src/index.ts (5)
10-15: Great addition offinallyFactoryparameter to the function signature.The introduction of an optional
finallyFactoryparameter enhances the API by allowing execution of cleanup logic regardless of promise outcome, similar to the standard Promise.finally() behavior.
28-40: Improved error handling implementation.The refactored implementation effectively handles errors from both the input factory and promise chain. Using
fakeRejectPromise(err)maintains consistent behavior by properly propagating errors through the promise chain.
42-44: Clean implementation of finally functionality.Good approach using a dedicated helper function
promiseLikeFinallyto handle the finally logic. This keeps the main function clean while properly applying the finalization step.
57-57: Appropriate type signature enhancement.Updating
fakePromiseto acceptT | MaybePromiseLike<T>makes the function more flexible and consistent with its usage in the refactoredhandleMaybePromise.
170-174: Enhanced error handling infakeRejectPromise.The updated implementation properly handles rejection cases by checking if a rejection handler is provided before applying it.
🧰 Tools
🪛 Biome (1.9.4)
[error] 170-170: Do not add then to an object.
(lint/suspicious/noThenProperty)
.changeset/late-flies-bet.md (1)
1-5: Appropriate changeset description.The changeset correctly documents the feature addition and marks it as a minor version bump, which is appropriate for new functionality that doesn't break backward compatibility.
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: You used an adverb (‘finally’) instead of an adjective, or a noun (‘callback’) instead of another adjective.
Context: ...se-helpers': minor --- Allow to pass a finally callback tohandleMaybePromise(A_RB_NN)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/promise-helpers/src/index.ts (1)
10-13: Good addition of theisActualPromiseutility function.This new helper properly checks for all three Promise method properties, distinguishing between general Promise-like objects and fully-featured Promise objects.
However, the implementation could be improved by using optional chaining for better safety:
- return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return maybePromise?.then && maybePromise?.catch && maybePromise?.finally;🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 71-71: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 182-182: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: unit / bun
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: server (ponyfill)
- GitHub Check: node-fetch (consumeBody)
🔇 Additional comments (8)
packages/promise-helpers/src/index.ts (8)
19-19: LGTM: The newfinallyFactoryparameter is consistently added to all overloads.This ensures type safety across all variants of
handleMaybePromise.Also applies to: 25-25, 31-31
33-38: Improved error handling with simplified initialization.The implementation is cleaner now, using
fakePromiseandfakeRejectPromisedirectly for initialization.
40-45: Good refactoring of result handling flow.The new approach with a single
result$variable improves code clarity. Usinginput$.then()with both success and error factories in a single call is more concise.
47-49: Good implementation of finalization logic.The finalization is correctly applied conditionally and leverages the native Promise
finallymethod.
51-59: Properly handles fake promises and rejections.The logic for checking and unwrapping fake promises remains intact, ensuring consistent behavior with the previous implementation.
180-186: Updated signature and implementation offakeRejectPromise.The return type change from
Promise<never>toPromise<T>is more flexible, and the implementation properly handles rejection cases.Similar to the previous comment, there's a linting error for the
thenproperty. Consider addressing this consistently.🧰 Tools
🪛 Biome (1.9.4)
[error] 182-182: Do not add then to an object.
(lint/suspicious/noThenProperty)
326-350: Well-implementedpromiseLikeFinallyutility function.This helper function correctly implements the finalization logic for both Promise objects with native
finallysupport and Promise-like objects without it.The implementation handles both success and error paths properly, ensuring the finalization callback is always executed while preserving the original promise result or error.
62-76:❓ Verification inconclusive
Improved
fakePromisefunction with better type support.The updated function handles both
MaybePromise<T>andMaybePromiseLike<T>appropriately, using the newisActualPromisecheck.The implementation of the
thenmethod in line 71 has a linting error. Consider restructuring to avoid the "no then property" warning.
🏁 Script executed:
#!/bin/bash # Check if there's a way to silence this specific linting error in the codebase grep -r "noThenProperty" --include="*.json" .Length of output: 45
Attention: Verify Linting Warning on
thenMethod Implementation
- The revised
fakePromisefunction now handles bothMaybePromise<T>andMaybePromiseLike<T>correctly using the newisActualPromisecheck, which is a solid improvement.- However, the inline implementation of the
thenmethod on line 71 currently triggers a lint warning regarding the "no then property" rule.- Our initial search for any configuration to suppress this rule (e.g., in JSON-based lint settings) returned no results, indicating that the warning isn’t being silenced globally.
- Action Required: Please consider refactoring the
thenmethod (line 71) to adhere to your linting guidelines—this might involve restructuring the promise-like object construction or delegating to a helper—to avoid this specific warning.🧰 Tools
🪛 Biome (1.9.4)
[error] 71-71: Do not add then to an object.
(lint/suspicious/noThenProperty)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/promise-helpers/src/index.ts (1)
10-13: Consider using optional chaining for safer property access.The
isActualPromisefunction correctly checks for the presence ofthen,catch, andfinallymethods, but you should use optional chaining to prevent potential runtime errors if the passed value is null or undefined.export function isActualPromise<T>(value: MaybePromiseLike<T>): value is Promise<T> { const maybePromise = value as any; - return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return !!maybePromise?.then && !!maybePromise?.catch && !!maybePromise?.finally; }🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 64-64: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 72-72: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 82-82: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 183-183: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
🔇 Additional comments (6)
packages/promise-helpers/src/index.ts (6)
15-32: Function signature extensions look good.The addition of the
finallyFactoryparameter to all overloads ofhandleMaybePromiseis consistent and well-typed.
33-60: Improved error handling and promise flow.The restructured implementation properly handles promise creation, error management, and the new finally functionality. The code now clearly separates input promise creation, output promise creation, and finally handling.
62-65: Type signature correctly updated.The function signature for
fakePromisenow properly reflects that it works with bothMaybePromise<T>andMaybePromiseLike<T>types, enhancing type inference.🧰 Tools
🪛 Biome (1.9.4)
[error] 64-64: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
70-77: Implementation correctly handles PromiseLike objects.The new implementation properly creates a Promise-like interface by utilizing only the
thenmethod of the provided PromiseLike object, addressing the TypeScript inference issue mentioned in the PR objectives.Note: There's a static analysis warning about
thenproperty, but this is a false positive as we're implementing a Promise-compatible interface.🧰 Tools
🪛 Biome (1.9.4)
[error] 72-72: Do not add then to an object.
(lint/suspicious/noThenProperty)
181-186: Type signature and implementation are now more flexible.The updated
fakeRejectPromisefunction correctly returnsPromise<T>instead ofPromise<never>, making it more versatile for error handling across different contexts.🧰 Tools
🪛 Biome (1.9.4)
[error] 183-183: Do not add then to an object.
(lint/suspicious/noThenProperty)
327-351: Well-implemented Promise.finally polyfill.The
promiseLikeFinallyfunction properly implements the Promise.finally behavior for objects that may not have native support. The implementation correctly handles both success and error cases, ensuring that the finally callback is always executed and the original result is properly propagated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/promise-helpers/src/index.ts (2)
10-13: Consider using optional chaining for more robust codeThe new
isActualPromisefunction correctly checks for the presence of all methods needed for an actual Promise, but could benefit from optional chaining to be more robust against edge cases.export function isActualPromise<T>(value: MaybePromiseLike<T>): value is Promise<T> { const maybePromise = value as any; - return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return maybePromise?.then && maybePromise?.catch && maybePromise?.finally; }🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
64-64: Consider using 'undefined' instead of 'void' for parameter typesThe static analyzer flagged the use of
voidas potentially confusing outside a return type or type parameter.-export function fakePromise(value: void): Promise<void>; +export function fakePromise(value: undefined): Promise<void>;🧰 Tools
🪛 Biome (1.9.4)
[error] 64-64: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 64-64: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 72-72: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 82-82: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 183-183: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: unit / node 18
- GitHub Check: server (undici)
- GitHub Check: server (ponyfill)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (native)
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (6)
packages/promise-helpers/src/index.ts (6)
15-32: LGTM! Consistent addition of finallyFactory parameter to function overloadsThe addition of the optional
finallyFactoryparameter to all overloads ofhandleMaybePromisemaintains consistent function signatures and improves the API by allowing finalization logic to run regardless of promise resolution status.
33-60: Improved error handling and promise flow with finallyFactory implementationThe restructured implementation now properly initializes and handles the input promise, applies error handling, and correctly implements the finalization step. The code structure is more robust and aligns well with Promise semantics.
62-77: Correct Promise behavior implementation with appropriate instanceof checkThe updated
fakePromisefunction now correctly checks for actual Promises using the new helper function and ensures proper propagation of Promise behavior.🧰 Tools
🪛 Biome (1.9.4)
[error] 64-64: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 72-72: Do not add then to an object.
(lint/suspicious/noThenProperty)
82-104: Enhanced error handling in promise then and finally methodsThe error handling for the
thenandfinallymethods has been improved with try-catch blocks to properly propagate errors through the promise chain.🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Do not add then to an object.
(lint/suspicious/noThenProperty)
181-216: Type signature improvement for fakeRejectPromiseThe updated generic type signature
Promise<T>(instead ofPromise<never>) provides more flexibility in how this function can be used while maintaining type safety.🧰 Tools
🪛 Biome (1.9.4)
[error] 183-183: Do not add then to an object.
(lint/suspicious/noThenProperty)
339-363: Well-implemented Promise.finally polyfillThe new
promiseLikeFinallyfunction correctly implements thefinallymethod for PromiseLike objects that don't have a native implementation. It properly handles both success and error paths, and preserves the semantics offinallyby returning the original value or rethrowing the original error after the callback executes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/promise-helpers/src/index.ts (3)
10-13: Great addition of theisActualPromisefunction for more precise Promise detection.However, consider enhancing robustness by checking for function types rather than just property existence:
export function isActualPromise<T>(value: MaybePromiseLike<T>): value is Promise<T> { const maybePromise = value as any; - return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return maybePromise && + typeof maybePromise.then === 'function' && + typeof maybePromise.catch === 'function' && + typeof maybePromise.finally === 'function'; }This ensures that the properties are actually callable methods.
🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
50-53: Consider replacingvoidwithundefinedin the parameter type.The static analyzer correctly flags that
voidis confusing outside a return type or a type parameter. Consider usingundefinedinstead:export function fakePromise<T>(value: MaybePromise<T>): Promise<T>; export function fakePromise<T>(value: MaybePromiseLike<T>): Promise<T>; -export function fakePromise(value: void): Promise<void>; +export function fakePromise(value: undefined): Promise<void>; export function fakePromise<T>(value: MaybePromiseLike<T>): Promise<T> {🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
327-351: Well-implementedpromiseLikeFinallyutility function.This function nicely handles the case where a PromiseLike object might not have a
finallymethod. However, the error handling could be more concise:return value.then( res => { const finallyRes = onFinally(); return isPromise(finallyRes) ? finallyRes.then(() => res) : res; }, err => { const finallyRes = onFinally(); - if (isPromise(finallyRes)) { - return finallyRes.then(() => { - throw err; - }); - } else { - throw err; - } + return isPromise(finallyRes) + ? finallyRes.then(() => { throw err; }) + : Promise.reject(err); }, );This refactoring makes the success and error branches more symmetrical and easier to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 52-52: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 60-60: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 70-70: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 171-171: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
- GitHub Check: server (undici)
- GitHub Check: alpha / snapshot
- GitHub Check: server (ponyfill)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (native)
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (4)
packages/promise-helpers/src/index.ts (4)
15-48: ThefinallyFactoryaddition improves the API by supporting cleanup operations.The restructured implementation is cleaner and more maintainable. The chain-based approach in line 33 improves readability, and the conditional handling of the
finallyFactoryis well-implemented.
54-65: ImprovedfakePromisehandling of promise-like objects.The implementation now correctly handles actual Promises, PromiseLike objects, and regular values. The delegation pattern for PromiseLike objects in lines 58-65 is well-implemented.
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Do not add then to an object.
(lint/suspicious/noThenProperty)
70-95: Improved error handling infakePromisemethods.The addition of try-catch blocks in the
thenandfinallymethods is a significant improvement, making the fake promise robust against synchronous errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Do not add then to an object.
(lint/suspicious/noThenProperty)
169-204: Generics improvement and better error handling infakeRejectPromise.Changing the return type from
Promise<never>toPromise<T>makes the function more flexible. The improved error handling with try-catch blocks in the Promise methods is also a valuable enhancement.🧰 Tools
🪛 Biome (1.9.4)
[error] 171-171: Do not add then to an object.
(lint/suspicious/noThenProperty)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/promise-helpers/src/index.ts (1)
10-13: Consider using optional chaining for safer property accessThe implementation of
isActualPromiseshould use optional chaining to avoid potential errors when accessing properties onnullorundefinedvalues.export function isActualPromise<T>(value: MaybePromiseLike<T>): value is Promise<T> { const maybePromise = value as any; - return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return Boolean(maybePromise?.then && maybePromise?.catch && maybePromise?.finally); }🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 44-44: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 52-52: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 62-62: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 163-163: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / deno
- GitHub Check: unit / bun
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: server (undici)
- GitHub Check: alpha / snapshot
- GitHub Check: prettier
- GitHub Check: server (ponyfill)
- GitHub Check: esm
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: lint
- GitHub Check: server (native)
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/promise-helpers/src/index.ts (9)
15-40: Great implementation of the finally callback featureThe updated
handleMaybePromisefunction now properly supports afinallyFactoryparameter which executes after the promise is settled, regardless of success or failure. The implementation is clean and follows promise semantics correctly.
42-57: Enhanced type safety and promise handlingThe updated
fakePromisefunction signatures and implementation correctly return aPromise<T>instead of just aPromiseLike<T>. The use ofisActualPromisehelps detect and handle true promises properly.🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 52-52: Do not add then to an object.
(lint/suspicious/noThenProperty)
65-68: Improved error handling with try-catchThe error handling in
fakePromise.thenis now safer with the use of try-catch blocks, which aligns with the PR objective to make the functions safe against exceptions.
77-84: Proper error handling in finally callbackThe addition of try-catch in the
finallymethod ensures that errors in the callback are properly converted to rejected promises, which improves the overall robustness of the library.
161-170: Enhanced type flexibility in fakeRejectPromiseThe updated signature of
fakeRejectPromisenow returnsPromise<T>instead ofPromise<never>, allowing for more flexible error handling. The implementation now also properly handles errors that might occur during rejection handling.🧰 Tools
🪛 Biome (1.9.4)
[error] 163-163: Do not add then to an object.
(lint/suspicious/noThenProperty)
175-179: Improved error propagation in catch methodThe try-catch block in the
catchmethod ensures proper error handling when callbacks throw exceptions, making the function safer to use.
185-189: Safe finally callback executionThe try-catch block in the
finallymethod ensures proper error handling when callbacks throw exceptions, making the function more robust.
319-343: Well-implementedpromiseLikeFinallyutilityThis new utility function correctly implements the
finallybehavior for PromiseLike objects that don't natively support it. It handles both success and error cases appropriately, ensuring that:
- The
onFinallycallback is always called- The original resolved value is preserved
- Original errors are re-thrown after the callback executes
345-355: Clean implementation ofunfakePromiseThe
unfakePromisefunction effectively converts fake promises back to their original values and handles error propagation correctly. This function simplifies the implementation ofhandleMaybePromiseby abstracting the conversion logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/promise-helpers/src/index.ts (6)
11-14: Consider using optional chaining for safer property accessThe
isActualPromisefunction checks for the presence of Promise methods, but it could benefit from optional chaining to avoid potential null/undefined errors when accessing properties.export function isActualPromise<T>(value: MaybePromiseLike<T>): value is Promise<T> { - const maybePromise = value as any; - return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + const maybePromise = value as any; + return Boolean(maybePromise?.then && maybePromise?.catch && maybePromise?.finally); }🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
43-58: RemovenoThenPropertylinting issues infakePromiseThe static analysis is flagging the use of
thenas a property name. While this is necessary for Promise implementation, consider adding a linting exception comment to clarify this is intentional.export function fakePromise<T>(value: MaybePromiseLike<T>): Promise<T> { if (value && isActualPromise(value)) { return value; } if (isPromise(value)) { return { + // eslint-disable-next-line no-restricted-syntax then: (resolve, reject) => fakePromise(value.then(resolve, reject)), catch: reject => fakePromise(value.then(res => res, reject)), finally: cb => fakePromise(cb ? promiseLikeFinally(value, cb) : value), [Symbol.toStringTag]: 'Promise', }; }🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 53-53: Do not add then to an object.
(lint/suspicious/noThenProperty)
63-70: Simplify error handling inthenmethodThe current implementation has a try/catch block that just rethrows the error, which could be simplified.
then(resolve) { if (resolve) { - try { - return fakePromise(resolve(value)); - } catch (err) { - throw err; - // return fakeRejectPromise(err); - } + return fakePromise(resolve(value)); } return this; },🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 63-63: Do not add then to an object.
(lint/suspicious/noThenProperty)
77-90: Simplify error handling infinallymethodSimilar to the
thenmethod, the error handling here can be simplified.finally(cb) { if (cb) { - try { - return fakePromise(cb()).then( - () => value, - () => value, - ); - } catch (err) { - throw err; - // return fakeRejectPromise(err); - } + return fakePromise(cb()).then( + () => value, + () => value, + ); } return this; },🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
164-184: Simplify error handling infakeRejectPromiseThe current implementation has try/catch blocks that just rethrow the error in multiple places, which could be simplified.
export function fakeRejectPromise<T>(error: unknown): Promise<T> { return { then(_resolve, reject) { if (reject) { - try { - return fakePromise(reject(error)); - } catch (err) { - throw err; - // return fakeRejectPromise(err); - } + return fakePromise(reject(error)); } return this; }, catch(reject: (error: unknown) => any) { if (reject) { - try { - return fakePromise(reject(error)); - } catch (err) { - throw err; - // return fakeRejectPromise(err); - } + return fakePromise(reject(error)); } return this; },🧰 Tools
🪛 Biome (1.9.4)
[error] 171-171: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 182-182: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 166-166: Do not add then to an object.
(lint/suspicious/noThenProperty)
188-198: Simplify error handling infinallymethod offakeRejectPromiseSimilar to other methods, the error handling here can be simplified.
finally(cb) { if (cb) { - try { - cb(); - } catch (err) { - throw err; - // return fakeRejectPromise(err); - } + cb(); } return this; },🧰 Tools
🪛 Biome (1.9.4)
[error] 193-193: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 13-13: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 45-45: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 68-68: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 53-53: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 63-63: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 85-85: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 171-171: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 182-182: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 193-193: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 166-166: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: server (undici)
- GitHub Check: server (ponyfill)
- GitHub Check: lint
- GitHub Check: server (native)
🔇 Additional comments (3)
packages/promise-helpers/src/index.ts (3)
16-40: LGTM: Well-structured addition offinallyFactoryparameterThe addition of the
finallyFactoryparameter tohandleMaybePromiseis well-implemented. The code correctly handles the finally callback regardless of whether the promise resolves or rejects.
325-349: LGTM: Well-implementedpromiseLikeFinallyfunctionThe
promiseLikeFinallyfunction correctly implements thefinallyfunctionality for Promise-like objects, ensuring the finalization logic runs regardless of the promise outcome.
351-361: LGTM: Clean implementation ofunfakePromiseThe
unfakePromisefunction elegantly unwraps fake promises to their original values, making the code inhandleMaybePromisecleaner and more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/promise-helpers/src/index.ts (4)
11-14: Use optional chaining for more robust property checksThe
isActualPromisefunction checks multiple properties of the potential promise object. Using optional chaining would make this code more robust against edge cases.export function isActualPromise<T>(value: MaybePromiseLike<T>): value is Promise<T> { const maybePromise = value as any; - return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return !!maybePromise?.then && !!maybePromise?.catch && !!maybePromise?.finally; }🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
56-71: Consider simplifying the fakePromise overloadsThe first two overloads could potentially be combined since they both return
Promise<T>.-export function fakePromise<T>(value: MaybePromise<T>): Promise<T>; -export function fakePromise<T>(value: MaybePromiseLike<T>): Promise<T>; +export function fakePromise<T>(value: MaybePromiseLike<T>): Promise<T>; export function fakePromise(value: void): Promise<void>;🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 66-66: Do not add then to an object.
(lint/suspicious/noThenProperty)
76-106: Remove unnecessary catch clauses that only rethrow errorsThere are several instances where a catch clause simply rethrows the error without additional handling, making the try/catch redundant. The commented-out code suggests the previous behavior involved returning a fake rejected promise.
then(resolve) { if (resolve) { - try { return fakePromise(resolve(value)); - } catch (err) { - throw err; - // return fakeRejectPromise(err); - } } return this; },Apply the same pattern to the other similar instances in the file (lines 90-101, 179-187, 190-198, 201-209).
🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 98-98: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 76-76: Do not add then to an object.
(lint/suspicious/noThenProperty)
234-236: Update mapMaybePromise to use the finallyFactory parameter
mapMaybePromiseis a wrapper aroundhandleMaybePromise, but it doesn't forward thefinallyFactoryparameter to it. Consider updating its signature to maintain API compatibility.export function mapMaybePromise<TInput, TOutput>( input: MaybePromiseLike<TInput>, onSuccess: (value: TInput) => MaybePromiseLike<TOutput>, onError?: (err: any) => MaybePromiseLike<TOutput>, + finallyFactory?: () => MaybePromiseLike<void>, ): MaybePromiseLike<TOutput> { - return handleMaybePromise(() => input, onSuccess, onError); + return handleMaybePromise(() => input, onSuccess, onError, finallyFactory); }This change should also be applied to the other overloads of
mapMaybePromise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 13-13: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 58-58: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 81-81: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 66-66: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 76-76: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 98-98: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 184-184: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 195-195: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 206-206: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 179-179: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: esm
- GitHub Check: unit / node 22
- GitHub Check: unit / bun
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: lint
- GitHub Check: server (undici)
- GitHub Check: alpha / snapshot
- GitHub Check: server (ponyfill)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (native)
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/promise-helpers/src/index.ts (5)
16-33: LGTM! Good addition of the finallyFactory parameterThe addition of the
finallyFactoryparameter to all overloads ofhandleMaybePromiseis well-structured. This allows for executing finalization logic regardless of promise resolution status.
34-54: Clean restructuring of the handleMaybePromise function implementationThe restructured implementation makes the flow more explicit, with clear separation between input promise creation, error handling, and result handling. The addition of the
finallyfunctionality in lines 49-51 implements the core feature of this PR correctly.
177-215: Improved error handling in fakeRejectPromiseThe updated implementation of
fakeRejectPromisecorrectly ensures that all promise methods are properly defined and handles errors consistently. The signature change fromPromise<never>toPromise<T>makes the function more flexible for different use cases.🧰 Tools
🪛 Biome (1.9.4)
[error] 184-184: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 195-195: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 206-206: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 179-179: Do not add then to an object.
(lint/suspicious/noThenProperty)
338-362: Well-implemented promiseLikeFinally utility functionThe new
promiseLikeFinallyfunction is a good addition that provides a consistent way to add finalization logic to promises, with fallback behavior for promise-like objects that don't have a nativefinallymethod.
364-374: Clean implementation of unfakePromiseThe
unfakePromisefunction correctly handles both successful and rejected fake promises, either by returning their value or throwing their error. This simplifies the external API by hiding the internal implementation details of fake promises.
EmrysMyrddin
force-pushed
the
feature/promise-helpers/handle-maybe-promise-finnally
branch
from
647d37e to
06310a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/promise-helpers/src/index.ts (3)
10-13: Solid addition with a minor improvement opportunity.The
isActualPromisefunction effectively differentiates between promises and promise-likes by checking for all three standard Promise methods. This will be useful for more precise type checking in the codebase.Consider using optional chaining for a more robust implementation:
- return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return maybePromise?.then && maybePromise?.catch && maybePromise?.finally;🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
42-45: Improved function signature with a minor type issue.The signatures now properly handle both
MaybePromise<T>andMaybePromiseLike<T>, making the function more flexible.TypeScript prefers
undefinedovervoidfor parameter types:-export function fakePromise(value: void): Promise<void>; +export function fakePromise(value: undefined): Promise<void>;🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
46-58: Remove commented-out code.The commented-out code blocks should be removed as they add noise without providing value. If this code represents alternative implementations that were considered, it would be better documented in PR comments or commit messages rather than in the source code.
- // if (value && isActualPromise(value)) { - // return value; - // } - - // if (isPromise(value)) { - // return { - // then: (resolve, reject) => fakePromise(value.then(resolve, reject)), - // catch: reject => fakePromise(value.then(res => res, reject)), - // finally: cb => fakePromise(cb ? promiseLikeFinally(value, cb) : value), - // [Symbol.toStringTag]: 'Promise', - // }; - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 44-44: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 66-66: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 167-167: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- 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: esm
- GitHub Check: unit / node 18
- GitHub Check: lint
- GitHub Check: prettier
- GitHub Check: type check
- GitHub Check: alpha / snapshot
- GitHub Check: server (undici)
- GitHub Check: server (ponyfill)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (native)
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/promise-helpers/src/index.ts (8)
19-19: LGTM! ThefinallyFactoryparameter is a good addition.Adding the optional
finallyFactoryparameter to all overloads ofhandleMaybePromiseproperly implements the main objective of this PR. The typing is appropriate and maintains consistency across all signature variants.Also applies to: 25-25, 31-31
33-39: Excellent refactoring of the implementation.The implementation has been elegantly simplified by leveraging
fakePromisefor chaining, which handles both synchronous and asynchronous cases. The conditional application offinallyFactoryis clean, and usingunfakePromiseat the end ensures that we maintain the expected return type.
59-61: Good simplification of the Promise handling logic.Directly returning the Promise when the input is already a Promise-like object simplifies the code and improves performance by avoiding unnecessary wrapping.
68-72: Excellent error handling improvements.Adding proper try-catch blocks to handle exceptions in callbacks is a significant improvement that makes the promise implementation more robust. Using
fakeRejectPromiseto propagate errors maintains the expected Promise behavior chain.Also applies to: 81-88
165-165: Improved type signature for better generic support.Changing the return type from
Promise<never>toPromise<T>makes the function more versatile and enables better type inference when used in generic contexts. This aligns with the PR's goal of making promise utilities more flexible.
167-174: Enhanced error handling in reject promise implementation.The improved implementation with try-catch blocks ensures that exceptions in callbacks are properly caught and propagated through the promise chain, making the fake promise implementation more robust and closer to native Promise behavior.
Also applies to: 179-183, 189-193
🧰 Tools
🪛 Biome (1.9.4)
[error] 167-167: Do not add then to an object.
(lint/suspicious/noThenProperty)
323-347: Well-implementedfinallypolyfill for Promise-like objects.The
promiseLikeFinallyfunction is a solid implementation that correctly handles both success and error cases, ensuring that thefinallycallback is executed in both scenarios while preserving the original promise's resolution value or rejection reason.
349-359: Great utility for promise unwrapping.The
unfakePromisefunction elegantly handles both fake promises and fake rejected promises, extracting their values or propagating errors appropriately. This supports the refactored implementation ofhandleMaybePromiseand helps maintain proper types throughout the promise chain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/promise-helpers/src/index.ts (2)
10-13: Enhance robustness with optional chainingThe implementation of
isActualPromisecould be improved by using optional chaining to better handle potential null or undefined values.export function isActualPromise<T>(value: MaybePromiseLike<T>): value is Promise<T> { const maybePromise = value as any; - return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return !!maybePromise?.then && !!maybePromise?.catch && !!maybePromise?.finally; }🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
42-58: Remove commented-out codeThe file contains a significant block of commented-out code that appears to be an alternative implementation that was considered but not used. This should be removed to improve code readability.
export function fakePromise<T>(value: MaybePromise<T>): Promise<T>; export function fakePromise<T>(value: MaybePromiseLike<T>): Promise<T>; export function fakePromise(value: void): Promise<void>; export function fakePromise<T>(value: MaybePromiseLike<T>): Promise<T> { - // if (value && isActualPromise(value)) { - // return value; - // } - - // if (isPromise(value)) { - // return { - // then: (resolve, reject) => fakePromise(value.then(resolve, reject)), - // catch: reject => fakePromise(value.then(res => res, reject)), - // finally: cb => fakePromise(cb ? promiseLikeFinally(value, cb) : value), - // [Symbol.toStringTag]: 'Promise', - // }; - // } -🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 44-44: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 66-66: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 171-171: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
- GitHub Check: alpha / snapshot
- GitHub Check: server (undici)
- GitHub Check: server (ponyfill)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (native)
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (6)
packages/promise-helpers/src/index.ts (6)
15-37: Good addition offinallyFactoryparameterThe addition of the
finallyFactoryparameter to thehandleMaybePromisefunction aligns well with the standard Promise API, allowing for cleanup operations regardless of promise resolution status.
33-39: Approve elegant implementation changesThe new implementation is more concise and effectively leverages the Promise API. The use of
fakePromise()as a starting point, combined with proper handling of thefinallyFactory, results in cleaner code.
165-204: Approve improved error handling infakeRejectPromiseThe enhanced error handling in
fakeRejectPromisemakes the function more resilient. The implementation now correctly propagates errors and maintains Promise behavior consistency.🧰 Tools
🪛 Biome (1.9.4)
[error] 171-171: Do not add then to an object.
(lint/suspicious/noThenProperty)
327-351: Well-implementedpromiseLikeFinallyutility functionThe new
promiseLikeFinallyfunction elegantly handles both standard Promises with nativefinallysupport and PromiseLike objects without it. The implementation correctly preserves the Promise chain behavior.
353-363: Approve the newunfakePromiseutilityThe
unfakePromisefunction effectively "unwraps" fake Promises, maintaining synchronous behavior where possible and avoiding unnecessary Promise wrapping. This helps optimize performance by reducing unnecessary asynchronous operations.
59-94:✅ Verification successful
Approve improved error handling in
fakePromiseThe implementation now correctly handles errors that might occur during promise resolution by using try-catch blocks in the
thenandfinallymethods. This makes the function more robust.
🏁 Script executed:
#!/bin/bash # Check for any instances where fakePromise is used in the codebase to make sure # the new error handling doesn't introduce regressions rg -A 3 "fakePromise\(" --type tsLength of output: 15336
Robust
fakePromiseError Handling Verified – No Regressions DetectedThe updated implementation in
packages/promise-helpers/src/index.tsnow wraps the resolution logic in both thethenandfinallymethods with try-catch blocks, ensuring that any errors are properly caught and redirected viafakeRejectPromise. A repository-wide search forfakePromiseusage confirms that this change is isolated and does not introduce regressions in other modules.
- The try-catch in
thenensures that any error during resolution is handled.- The try-catch in
finallyappropriately wraps callback execution and chains resolution.Overall, the improvements enhance robustness without affecting downstream usage.
🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Do not add then to an object.
(lint/suspicious/noThenProperty)
EmrysMyrddin
force-pushed
the
feature/promise-helpers/handle-maybe-promise-finnally
branch
from
7ee193a to
f1b9fcc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/promise-helpers/src/index.ts (1)
10-13: Use optional chaining for safer object property accessThe
isActualPromisefunction correctly checks for all required Promise methods, but could be more robust against null or undefined values by using optional chaining.export function isActualPromise<T>(value: MaybePromiseLike<T>): value is Promise<T> { const maybePromise = value as any; - return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return !!maybePromise?.then && !!maybePromise?.catch && !!maybePromise?.finally; }🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/src/index.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 44-44: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 52-52: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 62-62: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 163-163: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: server (undici)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: e2e / azure-function
- GitHub Check: server (ponyfill)
- GitHub Check: alpha / snapshot
- GitHub Check: e2e / aws-lambda
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: server (native)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/promise-helpers/src/index.ts (5)
15-40: Good implementation of thefinallyFactoryparameterThe updated
handleMaybePromisefunction now properly supports a finalization step through thefinallyFactoryparameter. The refactored implementation effectively chains promises and handles thefinallycallback regardless of whether the promise resolves or rejects.
42-91: EnhancedfakePromisenow handles both promise types and error casesThe updated
fakePromisefunction is more robust and versatile. It now:
- Returns actual Promise objects directly
- Creates a proper promise wrapper for PromiseLike objects
- Includes proper error handling with try/catch blocks
This aligns with the PR objectives to make promise helpers safer against exceptions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 52-52: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 62-62: Do not add then to an object.
(lint/suspicious/noThenProperty)
161-196: RevisedfakeRejectPromiseimplementationThe updated function signature and implementation are now more consistent with the intended use. The commented-out code indicates the deliberate removal of promise forwarding in the
thenmethod, which aligns with the PR objectives.Error handling has been improved throughout the implementation to catch exceptions and maintain the rejected state.
🧰 Tools
🪛 Biome (1.9.4)
[error] 163-163: Do not add then to an object.
(lint/suspicious/noThenProperty)
319-343: Well-implementedpromiseLikeFinallypolyfillThe new
promiseLikeFinallyfunction correctly implements thefinallyfunctionality for PromiseLike objects that might not support it natively. It properly handles both success and error cases, ensuring the finalization callback is always executed while preserving the original result or error.
345-355: Clean implementation ofunfakePromiseThe new
unfakePromisefunction completes the abstraction by converting fake promises back to their original values or throwing stored errors. This creates a clean, symmetrical API for working with promises throughout the library.
EmrysMyrddin
force-pushed
the
feature/promise-helpers/handle-maybe-promise-finnally
branch
from
f1b9fcc to
c964a81
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/promise-helpers/src/index.ts (2)
12-15: Use optional chaining for safer property access.The
isActualPromisefunction should use optional chaining to safely check for promise-like properties.export function isActualPromise<T>(value: MaybePromiseLike<T>): value is Promise<T> { const maybePromise = value as any; - return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return maybePromise?.then && maybePromise?.catch && maybePromise?.finally; }🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
44-94: EnhancedfakePromisewith better error handling and type support.The implementation of
fakePromisehas been significantly improved:
- It now properly handles both
MaybePromise<T>andMaybePromiseLike<T>types- It includes proper error handling with try/catch blocks for exception safety
- It properly implements the Promise interface including the finally method
- It uses the Symbol for identification
The static analysis tool flagged line 46 with "void is confusing outside a return type or a type parameter." Consider using
undefinedinstead.export function fakePromise<T>(value: MaybePromise<T>): Promise<T>; export function fakePromise<T>(value: MaybePromiseLike<T>): Promise<T>; -export function fakePromise(value: void): Promise<void>; +export function fakePromise(value: undefined): Promise<void>; export function fakePromise<T>(value: MaybePromiseLike<T>): Promise<T> {🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 54-54: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 64-64: 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 (2)
packages/promise-helpers/src/index.ts(4 hunks)packages/promise-helpers/tests/handleMaybePromise.spec.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 14-14: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 54-54: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 166-166: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / bun
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: server (undici)
- GitHub Check: lint
- GitHub Check: alpha / snapshot
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (ponyfill)
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: server (native)
🔇 Additional comments (7)
packages/promise-helpers/tests/handleMaybePromise.spec.ts (2)
1-3: Good choice of imports from Jest globals.The test file correctly imports Jest testing utilities from '@jest/globals' instead of from 'node:test', which aligns with the Jest testing framework being used.
5-291: Comprehensive test coverage for the newfinallycallback functionality.The test suite thoroughly covers:
- Synchronous and asynchronous scenarios
- Success and error cases
- Promise, sync function, and fake promise handling
- Chaining behavior with different combinations
- Error propagation
- Proper validation that
finallycallback is always calledThis level of test coverage will effectively prevent regressions and ensure the stability of the new functionality.
packages/promise-helpers/src/index.ts (5)
4-5: Good practice using a symbol constant for fake promise identification.Creating a named constant for the symbol improves maintainability and prevents magic strings.
17-42: Elegant implementation of thefinallycallback.The overloaded signatures and implementation of
handleMaybePromisenow properly support thefinallyFactoryparameter. The implementation is clean and straightforward:
- It creates a fake promise chain with the input, success, and error factories
- It conditionally applies the
finallyFactoryif provided- It returns the unwrapped result using
unfakePromiseThis is a well-designed enhancement to the function's capabilities.
164-199: EnhancedfakeRejectPromisewith better error handling.The implementation of
fakeRejectPromisehas been improved with proper error handling in try/catch blocks, making it more robust against exceptions. It now also uses the Symbol-based identification approach.The static analysis warnings about "Do not add then to an object" can be safely ignored in this case, as creating objects with
thenmethods is the intended behavior for fake promises.🧰 Tools
🪛 Biome (1.9.4)
[error] 166-166: Do not add then to an object.
(lint/suspicious/noThenProperty)
323-347: UsefulpromiseLikeFinallyutility for non-standard promises.This is a well-implemented utility function that adds
.finally()functionality forPromiseLikeobjects that don't have it natively. It handles both the success and error paths correctly, ensuring the finally callback is always executed.
349-359: Clean implementation ofunfakePromisefor unwrapping fake promises.The
unfakePromisefunction is straightforward and effectively handles both fake resolved promises and fake rejected promises. It's a useful utility that simplifies the rest of the codebase by centralizing the fake promise unwrapping logic.
ardatan
force-pushed
the
feature/promise-helpers/handle-maybe-promise-finnally
branch
from
2be6a2a to
d44bf68
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/promise-helpers/src/index.ts (1)
12-15: Consider using optional chaining for more robust type checking.The function correctly checks for the presence of Promise methods, but could be improved with optional chaining.
- return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return maybePromise?.then && maybePromise?.catch && maybePromise?.finally;🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/late-flies-bet.md(1 hunks)packages/promise-helpers/src/index.ts(4 hunks)packages/promise-helpers/tests/handleMaybePromise.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/promise-helpers/tests/handleMaybePromise.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 14-14: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 54-54: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 166-166: Do not add then to an object.
(lint/suspicious/noThenProperty)
🪛 LanguageTool
.changeset/late-flies-bet.md
[grammar] ~5-~5: You used an adverb (‘finally’) instead of an adjective, or a noun (‘callback’) instead of another adjective.
Context: ...se-helpers': minor --- Allow to pass a finally callback to handleMaybePromise
(A_RB_NN)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: prettier
- GitHub Check: server (undici)
- GitHub Check: lint
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: esm
- GitHub Check: alpha / snapshot
- GitHub Check: server (ponyfill)
- GitHub Check: server (native)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/promise-helpers/src/index.ts (8)
4-5: Good addition of a constant for the symbol name.Using a constant for the Symbol name improves maintainability and ensures consistency throughout the codebase.
17-34: Successfully added the finallyFactory parameter to all overloads.The function signatures have been correctly updated to include the finallyFactory parameter, maintaining consistency across all overloads.
35-42: Improved implementation with better promise chaining.The refactored implementation is more elegant, using
fakePromise()as a starting point for the chain and properly applying the finallyFactory when provided.
44-60: Enhanced type support and early promise detection.The function now accepts more flexible types and properly handles actual Promise objects early. The check for
isActualPromisebeforeisPromisecreates a clear hierarchy of promise recognition.🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 54-54: Do not add then to an object.
(lint/suspicious/noThenProperty)
61-94: Added robust error handling to prevent uncaught exceptions.The implementation now properly catches exceptions that might occur when executing the resolution or finally callbacks, enhancing reliability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
164-199: Improved fakeRejectPromise type and error handling.The function now:
- Returns
Promise<T>instead ofPromise<never>, allowing for more versatile error handling- Includes proper try-catch blocks around callback executions
- Uses consistent symbol identification
These improvements make the function more robust and align with the other changes.
🧰 Tools
🪛 Biome (1.9.4)
[error] 166-166: Do not add then to an object.
(lint/suspicious/noThenProperty)
323-347: Well-implemented finally callback for PromiseLike objects.This function fills an important gap by implementing the finally functionality for PromiseLike objects that may not have a built-in finally method. The implementation correctly handles both synchronous and asynchronous finally callbacks.
349-359: Good addition of unfakePromise for type safety.This function cleanly converts fake promises back to their original values, preserving the expected return type from
handleMaybePromise. The implementation properly handles both resolved and rejected fake promises..changeset/late-flies-bet.md (1)
1-6: Appropriate changeset for a minor version bump.The changeset correctly identifies this as a minor feature addition that adds new functionality without breaking existing code.
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: You used an adverb (‘finally’) instead of an adjective, or a noun (‘callback’) instead of another adjective.
Context: ...se-helpers': minor --- Allow to pass a finally callback tohandleMaybePromise(A_RB_NN)
ardatan
force-pushed
the
feature/promise-helpers/handle-maybe-promise-finnally
branch
from
fe5f72e to
1eca918
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/promise-helpers/src/index.ts (1)
12-15: Consider using optional chaining for safer property access.The current implementation might throw if
maybePromiseisnullorundefined. Consider using optional chaining to make the code more robust.- return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return !!maybePromise?.then?.catch?.finally;🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
.changeset/late-flies-bet.md (1)
1-6: Clear and concise changeset description.The changeset correctly describes the new feature of adding a finally callback to
handleMaybePromise. Consider rephrasing slightly for better grammar: "Allow passing a finally callback tohandleMaybePromise".🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: You used an adverb (‘finally’) instead of an adjective, or a noun (‘callback’) instead of another adjective.
Context: ...se-helpers': minor --- Allow to pass a finally callback tohandleMaybePromise(A_RB_NN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/late-flies-bet.md(1 hunks)packages/promise-helpers/src/index.ts(4 hunks)packages/promise-helpers/tests/handleMaybePromise.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/promise-helpers/tests/handleMaybePromise.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 14-14: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 54-54: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 166-166: Do not add then to an object.
(lint/suspicious/noThenProperty)
🪛 LanguageTool
.changeset/late-flies-bet.md
[grammar] ~5-~5: You used an adverb (‘finally’) instead of an adjective, or a noun (‘callback’) instead of another adjective.
Context: ...se-helpers': minor --- Allow to pass a finally callback to handleMaybePromise
(A_RB_NN)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: prettier
- GitHub Check: esm
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/promise-helpers/src/index.ts (9)
4-4: Good addition of a symbol name constant.Using a symbol with a meaningful name helps standardize the identification of fake promises across the codebase.
17-42: Well-structured implementation of thefinallycallback.The addition of the
finallyFactoryparameter to all overloads ofhandleMaybePromiseis consistent and provides a useful way to execute code after the promise settles, regardless of its outcome. The implementation is concise and correctly applies thefinallyFactoryonly when provided.
44-47: Type enhancement forfakePromisefunction signatures.The function signatures have been improved to accept both
MaybePromise<T>andMaybePromiseLike<T>, enhancing type safety and flexibility.🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
48-59: Good improvement in handling real promises and promise-like objects.The revised implementation properly differentiates between actual promises and promise-like objects, with separate handling paths for each. The chaining through
fakePromiseensures consistent behavior.🧰 Tools
🪛 Biome (1.9.4)
[error] 54-54: Do not add then to an object.
(lint/suspicious/noThenProperty)
64-93: Enhanced error handling in the fake promise implementation.The addition of try-catch blocks in the
thenandfinallymethods ensures that exceptions are properly caught and converted to rejected promises, making the implementation more robust.🧰 Tools
🪛 Biome (1.9.4)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
164-199: Improved error handling infakeRejectPromise.The enhanced implementation now properly catches exceptions in both
thenandcatchmethods, ensuring they are correctly converted to rejected promises. This makes the entire promise handling system more resilient to unexpected errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 166-166: Do not add then to an object.
(lint/suspicious/noThenProperty)
316-321: Updated symbol-based identification for fake promises.The implementation now uses the standardized symbol to identify fake resolved and rejected promises, which is more maintainable than using string literals directly.
323-347: Well-implementedpromiseLikeFinallyfunction.This utility function provides a clean implementation of the
finallyfunctionality for promise-like objects that may not have a nativefinallymethod. The implementation correctly preserves both successful results and errors after the finally callback executes.
349-359: Clean implementation ofunfakePromise.The function properly unwraps fake promises to either return their value or throw their stored error. This approach maintains the expected Promise semantics while allowing for synchronous execution when possible.
ardatan
force-pushed
the
feature/promise-helpers/handle-maybe-promise-finnally
branch
from
1eca918 to
19f8ff9
Compare
EmrysMyrddin
force-pushed
the
feature/promise-helpers/handle-maybe-promise-finnally
branch
from
bc59d11 to
938cc7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deno-jest.ts (1)
4-13:⚠️ Potential issueThe
it.eachimplementation has potential issues.While the parameterized testing implementation works for basic cases, there are several concerns:
- The function returns from within the loop which might cause only the last test case to be registered if multiple cases are provided.
- No error handling if a placeholder in the test name doesn't exist in the case object.
- Type safety could be improved beyond generic
object[].Apply this improved implementation:
it.each = (cases: object[]): typeof it => (name, runner) => { + // Store all test registrations to ensure all cases run + const testResults = []; for (const c of cases) { + let testName = name; Object.entries(c).forEach(([k, v]) => { - name = name.replaceAll(k, v); + testName = testName.replaceAll(k, String(v)); }); - return it(name, runner); + testResults.push(it(testName, runner)); } + return testResults[testResults.length - 1] || it(name, () => { throw new Error('No test cases were executed') }); };
🧹 Nitpick comments (1)
packages/promise-helpers/src/index.ts (1)
12-15: Consider using optional chaining for better null safety.The function correctly checks for a "real" Promise by verifying the existence of all three Promise methods, but it could benefit from optional chaining.
- return maybePromise && maybePromise.then && maybePromise.catch && maybePromise.finally; + return maybePromise?.then && maybePromise?.catch && maybePromise?.finally;🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/late-flies-bet.md(1 hunks)deno-jest.ts(1 hunks)packages/promise-helpers/src/index.ts(4 hunks)packages/promise-helpers/tests/handleMaybePromise.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/promise-helpers/tests/handleMaybePromise.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/promise-helpers/src/index.ts
[error] 14-14: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 54-54: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 168-168: Do not add then to an object.
(lint/suspicious/noThenProperty)
🪛 LanguageTool
.changeset/late-flies-bet.md
[grammar] ~5-~5: You used an adverb (‘finally’) instead of an adjective, or a noun (‘callback’) instead of another adjective.
Context: ...se-helpers': minor --- Allow to pass a finally callback to handleMaybePromise
(A_RB_NN)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: unit / bun
- GitHub Check: unit / node 18
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
🔇 Additional comments (9)
packages/promise-helpers/src/index.ts (7)
4-5: Good addition of centralized symbol name constant.Using a constant for the symbol name is a good practice to prevent string duplication and ensure consistency across the codebase.
17-42: Great improvement to the handleMaybePromise implementation.The function has been enhanced to support a finallyFactory parameter, and the implementation has been elegantly simplified using the fakePromise/unfakePromise pattern. This creates a more maintainable and readable code structure.
44-94: Improved fakePromise implementation with better type handling and error management.The updated implementation:
- Properly handles different types of inputs with appropriate type signatures
- Adds proper error handling for exceptional cases
- Better matches the behavior of real Promises
- Uses the centralized symbol name for identification
These changes make the function more robust and consistent with the Promise specification.
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 54-54: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
166-201: Enhanced fakeRejectPromise with better error handling.The changes to fakeRejectPromise improve error handling by safely catching exceptions during reject callbacks. The generic type parameter
Tinstead ofneverprovides more flexibility in how the function can be used.🧰 Tools
🪛 Biome (1.9.4)
[error] 168-168: Do not add then to an object.
(lint/suspicious/noThenProperty)
317-323: Switched to using the centralized symbol name.Good improvement to use the newly introduced constant for symbol identification, which ensures consistency throughout the codebase.
325-349: Well-implemented promiseLikeFinally function.This function adds finally support for PromiseLike objects that don't natively support it, carefully handling both success and error cases and properly chaining the promises. The implementation correctly ensures that the finally callback executes regardless of resolution status.
351-361: Good addition of unfakePromise unwrapping function.This function cleanly extracts values from fake promises, either returning the value directly or throwing the error, which makes the handleMaybePromise implementation much cleaner.
.changeset/late-flies-bet.md (1)
1-6: Clear and concise changeset description.The changeset correctly identifies this as a minor version update, which is appropriate for adding a new feature that doesn't break existing functionality. The description clearly communicates the enhancement.
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: You used an adverb (‘finally’) instead of an adjective, or a noun (‘callback’) instead of another adjective.
Context: ...se-helpers': minor --- Allow to pass a finally callback tohandleMaybePromise(A_RB_NN)
deno-jest.ts (1)
14-16: The export reorganization looks good.Separating the
itexport from the others makes sense now that you've added theit.eachmethod, as it allows for a cleaner organization of exports.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/promise-helpers/tests/handleMaybePromise.spec.ts (4)
67-102: Consider splitting falsy value tests for better isolation.While the tests are correct, having multiple assertions in a single test makes it harder to identify which specific falsy value might cause a failure. Consider splitting into separate test cases for each falsy value type.
- it.each(cases)('when fake value is falsy', ({ input, output }) => { + it.each(cases)('when fake value is undefined', ({ input, output }) => { expect( handleMaybePromise( () => (input === 'fake' ? fakePromise(undefined) : undefined), res => (output === 'fake' ? fakePromise(undefined) : res), ), ).toBe(undefined); + }); + + it.each(cases)('when fake value is null', ({ input, output }) => { expect( handleMaybePromise( () => (input === 'fake' ? fakePromise(null) : null), res => (output === 'fake' ? fakePromise(null) : res), ), ).toBe(null); + });
125-132: There's a discrepancy in the test description.The test description mentions "fake promise" but the test is using a real Promise.reject.
- it('should call finally and allow chaining with a fake promise', async () => { + it('should call finally and allow chaining with a rejected promise', async () => {
191-203: Consider using async/await pattern for consistency.This test uses try/catch while other tests use the async/await pattern with expect().rejects. Consider using the same pattern for consistency.
- it('should call finally and allow throw if no error handler is given', async () => { - const throwingFn = () => { - throw 'error'; - }; - try { - handleMaybePromise(throwingFn, onSuccess, undefined, onFinally); - throw 'Error was not thrown'; - } catch (err) { - expect(err).toBe('error'); - } - expect(onFinally).toHaveBeenCalledTimes(1); - expect(onSuccess).not.toHaveBeenCalled(); + it('should call finally and allow throw if no error handler is given', () => { + const throwingFn = () => { + throw 'error'; + }; + expect(() => { + handleMaybePromise(throwingFn, onSuccess, undefined, onFinally); + }).toThrow('error'); + expect(onFinally).toHaveBeenCalledTimes(1); + expect(onSuccess).not.toHaveBeenCalled(); });
259-268: Consider using expect().toThrow for consistency.Similar to the previous comment, consider using expect().toThrow for consistency with other tests.
- it('should call finally and allow throw if no error handler is given', async () => { - try { - handleMaybePromise(() => fakeRejectPromise('error'), onSuccess, undefined, onFinally); - throw 'Error has not been thrown'; - } catch (err) { - expect(err).toBe('error'); - } - expect(onFinally).toHaveBeenCalledTimes(1); - expect(onSuccess).not.toHaveBeenCalled(); + it('should call finally and allow throw if no error handler is given', () => { + expect(() => { + handleMaybePromise(() => fakeRejectPromise('error'), onSuccess, undefined, onFinally); + }).toThrow('error'); + expect(onFinally).toHaveBeenCalledTimes(1); + expect(onSuccess).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/promise-helpers/tests/handleMaybePromise.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: e2e / azure-function
🔇 Additional comments (21)
packages/promise-helpers/tests/handleMaybePromise.spec.ts (21)
1-4: LGTM! Clean import setup.The file properly sets up ESLint directives and imports necessary testing utilities from '@jest/globals' and the functions under test from the source file.
5-14: Test case structure looks good.Good organization with nested describe blocks and a well-defined test matrix for different input/output combinations.
15-22: Effective testing of synchronous behavior.The test correctly verifies that
handleMaybePromisereturns expected values synchronously when using various combinations of regular values and fake promises.
24-38: Good error handling test.This test effectively verifies that
handleMaybePromisecorrectly processes errors from both synchronous throws and fake rejected promises.
40-65: Thorough testing of error propagation.The test matrix approach is excellent for covering multiple scenarios. This section verifies that errors from onSuccess callbacks (both thrown and returned as fake rejected promises) are properly propagated.
104-114: Good setup for promise testing with finally callback.The test setup properly initializes mock functions that return promises for testing the asynchronous behavior with the finally callback.
116-123: Good test for finally with successful promises.This test effectively verifies that the finally callback is called after successful promise resolution and that the correct value is returned.
134-140: Good test for rejection propagation.This test correctly verifies that the finally callback is called even when promises are rejected and no error handler is provided.
142-150: Good error propagation test with finally.Test effectively verifies that the finally callback is called even when onSuccess rejects.
152-160: Good test for onError rejection with finally.This test correctly verifies that the finally callback is called even when onError rejects.
163-172: Good setup for synchronous testing.The test correctly sets up mock functions for testing the synchronous behavior with the finally callback.
174-179: Good test for finally with synchronous success.This test effectively verifies that the finally callback is called after successful synchronous execution.
181-189: Good test for synchronous error handling with finally.This test correctly verifies that the finally callback is called even when synchronous functions throw errors.
205-215: Good test for onSuccess throws with finally.This test effectively verifies that the finally callback is called even when onSuccess throws an error.
217-227: Good test for onError throws with finally.This test correctly verifies that the finally callback is called even when onError throws an error.
230-239: Good setup for fake promises testing.The test correctly sets up mock functions that return fake promises for testing.
241-248: Good test for finally with fake promises.This test effectively verifies that the finally callback is called with fake promises and the correct value is returned.
250-257: Good test for rejection with fake promises.This test correctly verifies that the finally callback is called with rejected fake promises.
270-278: Good test for onSuccess rejection with fake promises.This test effectively verifies that the finally callback is called even when onSuccess returns a rejected fake promise.
280-288: Good test for onError rejection with fake promises.This test correctly verifies that the finally callback is called even when onError returns a rejected fake promise.
289-292: Overall test organization is excellent.The test suite is well-organized, thorough, and covers all the key scenarios for the new finally callback feature.
This PR aims to ease the use of maybe async flows with handling of
finally.It contains 4 things:
handleMaybePromisecan now take afinallyFactorythat is guaranteed to always run, whatever happens in success or failure factories.fakePromisepreviously could return aPromiseLikeinstead of aPromise(because of a weird unsound TS inference). It have been fix to know ponyfill Promise interface by only usingthenfunction of the providedPromiseLikeobject.fakeRejectPromiseno longer forward promises. Because it doesn't have much sense, this function should be used to fake a rejected promise, so the parameter should always be an error that will be encapsulated into a fake rejected promisefakeRejectPromiseandfakePromiseare now safe against exceptions. This makeshandleMaybePromisea bit obsolete, but it's way more easier to use classic promise chaining API than the callback style ofhandleMaybePromiseunfakePromisewhich turns a (possibly fake)Promiseinto aMaybePromiseby extracting value or throwing error if the promise is fake.