feat: support HTTP2 #518
feat: support HTTP2 #518
Conversation
WalkthroughThe recent changes introduce HTTP/2 support in the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Application as Application
participant HttpClient as HttpClient
participant HttpAgent as HttpAgent
participant Server as Server
Note over User, Application: Initial Client Setup
User->>Application: Create HttpClient with allowH2
Application->>HttpClient: New HttpClient({ allowH2: true })
HttpClient->>HttpAgent: Instantiate HttpAgent with allowH2
Note over Application, Server: Making a Request
Application->>HttpClient: Make HTTP request
HttpClient->>HttpAgent: Get Connection
HttpAgent->>Server: Connect (HTTP/2 if supported)
Server->>HttpAgent: Respond
HttpAgent->>HttpClient: Relay Response
HttpClient->>Application: Return Response
Application->>User: Display Results
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #518 +/- ##
==========================================
+ Coverage 97.85% 97.86% +0.01%
==========================================
Files 10 10
Lines 1586 1596 +10
Branches 255 256 +1
==========================================
+ Hits 1552 1562 +10
Misses 30 30
Partials 4 4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
src/HttpClient.ts (2)
Line range hint
364-364: Avoid using the delete operator for performance reasons.Using
deletecan lead to performance degradations due to changes in object shape. Consider setting the property toundefinedinstead.- delete headers['user-agent']; + headers['user-agent'] = undefined;
Line range hint
572-572: Correct the unsafe usage of optional chaining.Ensure that the evaluation does not throw a TypeError when the optional chain short-circuits with 'undefined'.
- if (context.history?) { + if (context?.history && context.history.length > 0) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- README.md (1 hunks)
- src/HttpAgent.ts (2 hunks)
- src/HttpClient.ts (2 hunks)
- test/HttpClient.test.ts (2 hunks)
Additional context used
LanguageTool
README.md
[misspelling] ~55-~55: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’. (EN_A_VS_AN)
Context: ... The URL to request, either a String or a Object that return by [url.parse](https...
[style] ~57-~57: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...ng - Request method, defaults toGET. Could beGET,POST,DELETEorPUT. Al...
[grammar] ~60-~60: The past participle is required after “to be”. (BE_VBP_IN)
Context: ...m_class_stream_readable) - Stream to be pipe to the remote. If set,dataand `cont...
[grammar] ~61-~61: There may an error in the verb form ‘be write’. (MD_BE_NON_VBP)
Context: ...e response stream. Responding data will be write to this stream andcallbackwill be c...
[style] ~63-~63: To form a complete sentence, be sure to include a subject or ‘there’. (MISSING_IT_THERE)
Context: ...tType*** String - Type of request data. Could bejson(Notes: not use `applicat...
[uncategorized] ~63-~63: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...ation/jsonhere). If it'sjson, will auto setContent-Type: application/json` header...
[style] ~64-~64: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...Type*** String - Type of response data. Could betextorjson. If it'stext, th...
[uncategorized] ~64-~64: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ... would be a parsed JSON Object and will auto setAccept: application/jsonheader. Defa...
[uncategorized] ~67-~67: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO)
Context: ... can usetimeout: 5000to tell urllib use same timeout on two phase or set them s...
[grammar] ~67-~67: Possible agreement error. The noun ‘phase’ seems to be countable. (CD_NN)
Context: ... to tell urllib use same timeout on two phase or set them seperately such as `timeout...
[misspelling] ~73-~73: Did you mean “yourself”? Remove the space in between. (YOUR_SELF_TO_YOURSELF)
Context: ...* Function - Format the redirect url by your self. Default isurl.resolve(from, to). ...
[misspelling] ~74-~74: Use ‘every thing’ if you want to emphasize that the things are separate. Usually, you can use “everything”. (EVERYTHING)
Context: ...n - Before request hook, you can change every thing here. - streaming Boolean - let...
[uncategorized] ~76-~76: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ... Acceptgzip, brresponse content and auto decode it, default istrue. - timing...
[misspelling] ~123-~123: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’. (EN_A_VS_AN)
Context: ...: 'world', }), });It would make a HTTP request like:bash POST / HTTP...
[uncategorized] ~203-~203: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...uest and response time in ms. -timing: timing object if timing enable. - `sock...
[uncategorized] ~204-~204: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ming object if timing enable. -socket: socket info ## Run test with debug log...
[misspelling] ~256-~256: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’. (EN_A_VS_AN)
Context: ...ad request' }); ``` ## Request through a http proxy export from [undici](https:...
Biome
src/HttpClient.ts
[error] 364-364: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 556-556: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 572-572: Unsafe usage of optional chaining. (lint/correctness/noUnsafeOptionalChaining)
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
Additional comments not posted (9)
src/HttpAgent.ts (2)
15-15: Addition ofallowH2property toHttpAgentOptionsThe addition of the
allowH2property to theHttpAgentOptionstype is crucial for enabling HTTP2 support. This change allows users to configure their HTTP agents to support HTTP2, aligning with the PR's objective to introduce HTTP2 support.
66-66: Inclusion ofallowH2in theconnectobjectThe constructor now includes the
allowH2option in theconnectconfiguration object. This change is essential for ensuring that the HTTP2 setting is respected when theHttpAgentis used. It is a necessary update to support the new functionality introduced by the PR.
[APROVED]test/HttpClient.test.ts (3)
3-3: Import ofsensitiveHeadersfromnode:http2This import is crucial for testing HTTP2 functionality, specifically to check for sensitive headers that are specific to HTTP2 communications. It is a necessary addition for the new tests introduced.
5-5: Updated imports inHttpClient.test.tsThe inclusion of
getGlobalDispatcherfrom../src/index.jsis essential for testing interactions with global dispatchers in the context of HTTP2. This change supports the new testing requirements introduced by the PR.
31-97: New test suite forallowH2optionThe new test suite specifically targets the
allowH2functionality. It includes tests to verify that HTTP2 is properly handled whenallowH2is set to true and that the standard HTTP/1.1 behavior remains unaffected whenallowH2is false. These tests are well-structured and crucial for ensuring the robustness of the new functionality.README.md (1)
212-226: Documentation for making HTTP2 requestsThe new section in the README file provides clear instructions on how to make HTTP2 requests by setting the
allowH2option. This documentation is well-written and crucial for helping users understand how to utilize the new HTTP2 support in the library.src/HttpClient.ts (3)
60-61: Approved addition ofallowH2property.The optional
allowH2property inClientOptionsis a clean implementation to support HTTP2, defaulting tofalsewhich is a safe default.
182-182: Proper handling ofallowH2in dispatcher configuration.The conditional checks and subsequent setting of
allowH2in the dispatcher configurations are correctly implemented, ensuring HTTP2 is enabled based on user configuration.Also applies to: 187-187, 192-192
Line range hint
556-556: Use optional chaining to simplify code.Changing to optional chaining can make the code cleaner and less prone to errors.
- if (context?.history) { + if (context.history?) {
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/HttpClient.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/HttpClient.test.ts
[skip ci] ## [4.1.0](v4.0.0...v4.1.0) (2024-06-27) ### Features * support HTTP2 ([#518](#518)) ([21d4260](21d4260))
closes #474
pick from #516
Summary by CodeRabbit
New Features
HttpClientwith the newallowH2option.getGlobalDispatcherfunction for managing global dispatchers.Documentation
HttpClient.Tests
allowH2option inHttpClient.Chores