fix: keep promise chains intact #4558
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on maintaining promise chains by ensuring that promise-returning functions properly return their promises instead of creating orphaned promises. The changes improve the async control flow by making promise chains chainable and preventing unhandled promise rejections.
- Adds explicit
returnstatements to promise chains that were previously orphaned - Refactors promise chain formatting for better readability and consistency
- Introduces a reusable
nopfunction to replace inline empty arrow functions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/mock/snapshot-agent.js | Reformats promise chain for better readability |
| lib/mock/mock-utils.js | Adds missing return statement to promise chain |
| lib/interceptor/cache.js | Introduces nop function and fixes promise chain returns |
| lib/dispatcher/pool-base.js | Adds missing return statement to Promise.all |
| lib/dispatcher/dispatcher-base.js | Reformats promise chains and removes unnecessary closures |
| lib/core/util.js | Adds return statements to controller methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return queueMicrotask(() => { | ||
| controller.close() | ||
| controller.byobRequest?.respond(0) |
There was a problem hiding this comment.
queueMicrotask doesn't return a value, so this return statement will return undefined. If a promise is expected, consider wrapping this in a Promise or removing the return statement.
| return queueMicrotask(() => { | |
| controller.close() | |
| controller.byobRequest?.respond(0) | |
| return new Promise(resolve => { | |
| queueMicrotask(() => { | |
| controller.close() | |
| controller.byobRequest?.respond(0) | |
| resolve() | |
| }) |
| const buf = Buffer.isBuffer(value) ? value : Buffer.from(value) | ||
| if (buf.byteLength) { | ||
| controller.enqueue(new Uint8Array(buf)) | ||
| return controller.enqueue(new Uint8Array(buf)) |
There was a problem hiding this comment.
controller.enqueue() typically doesn't return a meaningful value for chaining. This return statement may not provide the expected behavior in the promise chain.
| return controller.enqueue(new Uint8Array(buf)) | |
| controller.enqueue(new Uint8Array(buf)) | |
| return |
|
@metcoder95 |
|
I think it is ok. It doesnt make it worse. |
Disputable
This relates to...
Rationale
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status