Switch from Net::HTTP to HTTPX for communication with Node renderer#452
Switch from Net::HTTP to HTTPX for communication with Node renderer#452alexeyr-ci merged 14 commits intomasterfrom
Conversation
WalkthroughThe pull request introduces several significant updates to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (13)
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. 🪧 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 (
|
665f102 to
826d57b
Compare
| # :request_timeout | ||
| # :operation_timeout | ||
| # :keep_alive_timeout | ||
| # TODO: Do we want to add config for them? |
There was a problem hiding this comment.
Or set request_timeout (total time to write the request and read the response) instead of read_timeout for ssr_timeout?
There was a problem hiding this comment.
We can keep the same timeouts. Do you have a reason to set a timeout for the whole request instead of read_timeout?
There was a problem hiding this comment.
It depends on how we count time for SSR: the total time from RoRP, or only the time in the Node renderer? For Net::HTTP it just wasn't an option, I think.
826d57b to
3f43f2b
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
3f43f2b to
a388c59
Compare
a388c59 to
3952146
Compare
| end | ||
|
|
||
| config.before(:suite) do | ||
| ReactOnRailsPro::Request.reset_connection |
There was a problem hiding this comment.
Could do both in a single before hook.
3952146 to
f281944
Compare
| end | ||
| stream_response = @request_executor.call(send_bundle) | ||
| # stream_response.each may yield merged chunks, but the real chunks are separated by newlines. | ||
| stream_response.each_line do |chunk| |
There was a problem hiding this comment.
If the chunk separator is changed or if the chunk is allowed to contain new lines, how should we iterate over chunks in this case?
There was a problem hiding this comment.
We can use each instead of each_line to iterate over the (possibly merged?) chunks.
There was a problem hiding this comment.
Question because I don't know if HTTPX will actually merge chunks like that or if it keeps track of the original chunks.
There was a problem hiding this comment.
Merging chunks is no problem; we use a separator. FYI, the node-renderer already merges chunks when more than one chunk is generated simultaneously.
AbanoubGhadban
left a comment
There was a problem hiding this comment.
Good work. I just have some questions and a small change request
fc585d3 to
7912c52
Compare
0ae9bdf to
2535959
Compare
To prepare for HTTP/2 support, we need to switch to a new HTTP client library on the Ruby side.
Extracted from #392.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores