-
-
Couldn't load subscription status.
- Fork 672
chore: make client-h2.js more resilient #4564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the resilience and maintainability of the HTTP/2 client implementation by adding comprehensive JSDoc type annotations and making defensive programming improvements.
- Adds JSDoc type definitions and function documentation for better type safety
- Improves error handling in GOAWAY frame processing with null checks
- Updates variable declarations and comparisons for better code clarity
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Specifically, we do not verify the "valid" stream id. | ||
|
|
||
| const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, util.getSocketInfo(this[kSocket])) | ||
| const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, this[kSocket] && util.getSocketInfo(this[kSocket])) |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check this[kSocket] && will pass false to util.getSocketInfo() when socket is null/undefined, which may cause unexpected behavior. Consider using a ternary operator: this[kSocket] ? util.getSocketInfo(this[kSocket]) : null
| const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, this[kSocket] && util.getSocketInfo(this[kSocket])) | |
| const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, this[kSocket] ? util.getSocketInfo(this[kSocket]) : null) |
|
|
||
| /** @type {import('node:http2').ClientHttp2Stream} */ | ||
| let stream = null | ||
| let stream |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Variable stream is declared without initialization. Consider initializing it explicitly as let stream = undefined for clarity, or use the original null initialization if that was intentional.
| let stream | |
| let stream = undefined |
This relates to...
Rationale
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status