Add support for streaming rendered components using renderToPipeableStream#1633
Conversation
WalkthroughThis update enhances the Changes
Possibly related PRs
Suggested reviewers
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 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: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (9)
- lib/react_on_rails/helper.rb (5 hunks)
- lib/react_on_rails/react_component/render_options.rb (1 hunks)
- lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1 hunks)
- node_package/src/ReactOnRails.ts (2 hunks)
- node_package/src/serverRenderReactComponent.ts (4 hunks)
- node_package/src/types/index.ts (2 hunks)
- package.json (2 hunks)
- spec/dummy/config/webpack/alias.js (1 hunks)
- spec/dummy/config/webpack/webpackConfig.js (1 hunks)
Files skipped from review due to trivial changes (2)
- package.json
- spec/dummy/config/webpack/alias.js
Additional context used
Biome
node_package/src/types/index.ts
[error] 121-121: void is confusing inside a union type. (lint/suspicious/noConfusingVoidType)
Unsafe fix: Use undefined instead.
node_package/src/ReactOnRails.ts
[error] 110-110: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 117-117: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 185-185: The function should not return a value because its return type is void. (lint/correctness/noVoidTypeReturn)
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
Additional comments not posted (6)
spec/dummy/config/webpack/webpackConfig.js (1)
7-7: The fallback configuration for thestreammodule usingstream-browserifyis correctly set up to ensure compatibility across environments.lib/react_on_rails/react_component/render_options.rb (1)
106-108: Thestream?method is appropriately added to check for the streaming option. This supports the new feature of server-side streaming rendering.node_package/src/types/index.ts (2)
2-2: ImportingPassThroughfromstreamis correctly added to support the types used in the new streaming functions.
141-141: The addition ofstreamServerRenderedReactComponentto theReactOnRailsinterface correctly extends its capabilities to support streaming.node_package/src/serverRenderReactComponent.ts (1)
74-99: The implementation of streaming usingReactDOMServer.renderToPipeableStreamand handling the stream withPassThroughis correctly done. This is a crucial part of adding streaming support to the server rendering process.Also applies to: 209-249
lib/react_on_rails/helper.rb (1)
94-102: Proper implementation of streaming support instream_react_component.The method correctly sets up options for streaming and builds the result for server-streamed content. This is a crucial addition for supporting modern React streaming capabilities.
| if (throwJsErrors) { | ||
| throw e; | ||
| } | ||
|
|
||
| renderResult = stringToStream(handleError({ | ||
| e, | ||
| name, | ||
| serverSide: true, | ||
| })); |
There was a problem hiding this comment.
Consider enhancing error handling by adding more specific error messages or logging for different types of errors, especially in streaming contexts.
// Example: Add more specific error handling
if (e instanceof SpecificError) {
console.error(`Specific error occurred: ${e.message}`);
} else {
console.error(`General error in streaming: ${e}`);
}| @@ -1,10 +1,11 @@ | |||
| import type { ReactElement } from 'react'; | |||
| import type { PassThrough } from 'stream'; | |||
There was a problem hiding this comment.
Ensure correct implementation of streaming in streamServerRenderedReactComponent.
The implementation of streamServerRenderedReactComponent seems to recursively call itself, which will lead to a stack overflow error. This should be corrected to call the appropriate function that handles the streaming logic.
- return streamServerRenderedReactComponent(options);
+ return someInternalFunctionThatHandlesStreaming(options);Also applies to: 8-8, 245-252
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)
- lib/react_on_rails/helper.rb (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails/helper.rb
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
node_package/src/serverRenderReactComponent.ts (1)
171-176: Consider adding a brief explanation forPassThrough.Adding a comment explaining the usage of
PassThroughcan improve readability for future maintainers.const stringToStream = (str: string) => { // PassThrough is a type of stream that passes data through without transforming it. const stream = new PassThrough(); stream.push(str); stream.push(null); return stream; };
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- node_package/src/serverRenderReactComponent.ts (2 hunks)
Additional comments not posted (1)
node_package/src/serverRenderReactComponent.ts (1)
178-220: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
streamServerRenderedReactComponentare correctly implemented.Verification successful
Function usage verified.
The function
streamServerRenderedReactComponentis correctly implemented in the codebase.
node_package/src/ReactOnRails.ts: The methodstreamServerRenderedReactComponentcorrectly calls the imported function with the appropriate parameters.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `streamServerRenderedReactComponent` are correctly implemented. # Test: Search for the function usage. Expect: Only occurances of the new function. rg --type typescript -A 5 $'streamServerRenderedReactComponent'Length of output: 97
Script:
#!/bin/bash # Description: Verify all function calls to `streamServerRenderedReactComponent` are correctly implemented. # Test: Search for the function usage across all files. rg -A 5 'streamServerRenderedReactComponent'Length of output: 2000
Script:
#!/bin/bash # Description: Verify the context around the function usage in `node_package/src/ReactOnRails.ts`. # Test: Display lines around the function usage in `node_package/src/ReactOnRails.ts`. rg -A 10 -B 10 'streamServerRenderedReactComponent' node_package/src/ReactOnRails.tsLength of output: 1322
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
lib/react_on_rails/helper.rb (1)
104-122: Functionstream_react_component_asyncis not used in the codebase.The method
stream_react_component_asyncis defined but not called anywhere in the repository. Ensure that this function is integrated where necessary or remove it if it is not needed.
- File:
lib/react_on_rails/helper.rb- Line: 104
Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
stream_react_component_asyncare correctly implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `stream_react_component_async` are correctly implemented. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type ruby -A 5 $'stream_react_component_async'Length of output: 470
Script:
#!/bin/bash # Description: Verify all function calls to `stream_react_component_async` are correctly implemented. # Test: Search for the function usage across the entire codebase. rg 'stream_react_component_async'Length of output: 129
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lib/react_on_rails/helper.rb (4 hunks)
Additional comments not posted (2)
lib/react_on_rails/helper.rb (2)
394-419: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
build_react_component_result_for_server_streamed_contentare correctly implemented.Verification successful
Function usage verified!
The function
build_react_component_result_for_server_streamed_contentis correctly implemented and used within the codebase.
- The method definition and its single call are both located in
lib/react_on_rails/helper.rb.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `build_react_component_result_for_server_streamed_content` are correctly implemented. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type ruby -A 5 $'build_react_component_result_for_server_streamed_content'Length of output: 985
Script:
#!/bin/bash # Description: Verify all function calls to `build_react_component_result_for_server_streamed_content` are correctly implemented across the entire codebase. # Test: Search for the function usage in the entire codebase. Expect: Only occurrences of the new signature. rg --type ruby 'build_react_component_result_for_server_streamed_content'Length of output: 261
94-102: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
stream_react_componentare correctly implemented.Verification successful
The search results indicate that the
stream_react_componentmethod is used within thestream_react_component_asyncmethod. This usage appears to be correct. However, to be thorough, we need to ensure there are no other occurrences ofstream_react_componentin the codebase that could be impacted by the new changes.
Verified: The
stream_react_componentmethod is defined and used only within thelib/react_on_rails/helper.rbfile, specifically within thestream_react_component_asyncmethod. No other occurrences were found in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `stream_react_component` are correctly implemented. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type ruby -A 5 $'stream_react_component'Length of output: 1066
Script:
#!/bin/bash # Search for all occurrences of `stream_react_component` in the codebase to ensure correct implementation. rg 'stream_react_component'Length of output: 307
AbanoubGhadban
force-pushed
the
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
branch
from
00c9552 to
fbb1060
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .gitignore (1 hunks)
- lib/react_on_rails/helper.rb (4 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails/helper.rb
AbanoubGhadban
force-pushed
the
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
branch
from
fbb1060 to
7bb828e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (10)
- .gitignore (1 hunks)
- lib/react_on_rails/helper.rb (4 hunks)
- lib/react_on_rails/react_component/render_options.rb (1 hunks)
- lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1 hunks)
- node_package/src/ReactOnRails.ts (2 hunks)
- node_package/src/serverRenderReactComponent.ts (2 hunks)
- node_package/src/types/index.ts (2 hunks)
- package.json (2 hunks)
- spec/dummy/config/webpack/alias.js (1 hunks)
- spec/dummy/config/webpack/webpackConfig.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- .gitignore
- node_package/src/serverRenderReactComponent.ts
Additional comments not posted (10)
spec/dummy/config/webpack/alias.js (1)
7-7: Addition of alias forstreamtostream-browserify.This addition correctly maps the
streammodule tostream-browserify, ensuring compatibility with browser environments. Ensure that this change does not conflict with other aliases or module resolutions in your project.spec/dummy/config/webpack/webpackConfig.js (1)
7-7: Addition of fallback forstreamtostream-browserify.This addition correctly sets a fallback for the
streammodule to usestream-browserify, ensuring compatibility with browser environments. Ensure that this change does not conflict with other fallbacks or module resolutions in your project.package.json (2)
18-19: Upgrade@types/reactand@types/react-domto18.2.0.These upgrades align the type definitions with React 18, ensuring compatibility with the latest features and types. Verify that the rest of the codebase is compatible with these updates.
42-43: Upgradereactandreact-domto18.2.0.These upgrades bring in the latest features and performance improvements of React 18. Ensure that the rest of the codebase is compatible with these updates.
lib/react_on_rails/react_component/render_options.rb (1)
106-108: LGTM!The
stream?method correctly retrieves the value associated with the:stream?key from theoptionshash.node_package/src/types/index.ts (2)
2-2: LGTM!The import statement for
PassThroughfrom thestreammodule is correct and necessary for the new method.
141-141: LGTM!The method
streamServerRenderedReactComponentcorrectly returns aPassThroughstream and enhances the functionality of theReactOnRailsinterface.lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1)
96-99: LGTM! But remember to merge withexec_server_render_js.The method
exec_server_render_streaming_jscorrectly invokes theeval_streaming_jsmethod. The TODO comment indicates an intention to merge this method withexec_server_render_jsin the future.lib/react_on_rails/helper.rb (2)
94-102: LGTM!The
stream_react_componentfunction correctly merges options with astream?flag and callsinternal_react_component. The function also correctly invokesbuild_react_component_result_for_server_streamed_content.
104-122: LGTM!The
stream_react_component_asyncfunction correctly uses Ruby'sFiberto manage asynchronous streaming of React components. The function correctly creates a fiber that streams chunks of the component's output and yields the initial chunk.
| streamServerRenderedReactComponent(options: RenderParams): PassThrough { | ||
| return streamServerRenderedReactComponent(options); | ||
| }, |
There was a problem hiding this comment.
Ensure correct implementation of streaming in streamServerRenderedReactComponent.
The implementation of streamServerRenderedReactComponent seems to recursively call itself, which will lead to a stack overflow error. This should be corrected to call the appropriate function that handles the streaming logic.
- return streamServerRenderedReactComponent(options);
+ return someInternalFunctionThatHandlesStreaming(options);Committable suggestion was skipped due to low confidence.
| def build_react_component_result_for_server_streamed_content( | ||
| rendered_html_stream: required("rendered_html_stream"), | ||
| component_specification_tag: required("component_specification_tag"), | ||
| render_options: required("render_options") | ||
| ) | ||
| content_tag_options_html_tag = render_options.html_options[:tag] || "div" | ||
| # The component_specification_tag is appended to the first chunk | ||
| # We need to pass it early with the first chunk because it's needed in hydration | ||
| # We need to make sure that client can hydrate the app early even before all components are streamed | ||
| is_first_chunk = true | ||
| rendered_html_stream = rendered_html_stream.transform do |chunk| | ||
| if is_first_chunk | ||
| is_first_chunk = false | ||
| next <<-HTML | ||
| #{rails_context_if_not_already_rendered} | ||
| #{component_specification_tag} | ||
| <#{content_tag_options_html_tag} id="#{render_options.dom_id}">#{chunk}</#{content_tag_options_html_tag}> | ||
| HTML | ||
| end | ||
| chunk | ||
| end | ||
|
|
||
| rendered_html_stream.transform(&:html_safe) | ||
| # TODO: handle console logs | ||
| end |
There was a problem hiding this comment.
LGTM! But handle console logs.
The build_react_component_result_for_server_streamed_content function correctly handles the first chunk of the stream and ensures the client can hydrate the app early. The function also correctly marks the content as html_safe.
However, the TODO comment indicates that handling console logs is pending.
Do you want me to help with handling console logs or open a GitHub issue to track this task?
|
|
||
| def stream_react_component(component_name, options = {}) | ||
| options = options.merge(stream?: true) | ||
| result = internal_react_component(component_name, options) |
| # We need to pass it early with the first chunk because it's needed in hydration | ||
| # We need to make sure that client can hydrate the app early even before all components are streamed | ||
| is_first_chunk = true | ||
| rendered_html_stream = rendered_html_stream.transform do |chunk| |
There was a problem hiding this comment.
The StreamRequest and StreamDecorator classes allow us to transform the stream of http body chunks
| "prop-types": "^15.8.1", | ||
| "react": "^17.0.0", | ||
| "react-dom": "^17.0.0", | ||
| "react": "^18.2.0", |
There was a problem hiding this comment.
renderToReadableStream function is added in React 18
AbanoubGhadban
left a comment
AbanoubGhadban
left a comment
There was a problem hiding this comment.
Self-reviewed
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- jest.config.js (1 hunks)
- node_package/src/serverRenderReactComponent.ts (2 hunks)
- node_package/tests/ReactOnRails.test.js (1 hunks)
- node_package/tests/jest.setup.js (1 hunks)
Additional comments not posted (5)
jest.config.js (1)
4-4: Configuration Addition: setupFilesThe addition of the
setupFilesproperty to the Jest configuration is appropriate. This allows for the initialization of necessary settings before each test suite runs.node_package/tests/jest.setup.js (1)
1-13: Polyfills for TextEncoder and TextDecoderThe polyfills for
TextEncoderandTextDecoderare correctly implemented. The check to avoid redefining these globals if they are already defined is a good practice.node_package/tests/ReactOnRails.test.js (1)
22-30: Improved Test Setup and VerificationThe changes to programmatically create the
<div>element and set its ID and text content improve the clarity and accuracy of the test setup. This approach is more explicit and ensures the DOM is correctly set up before rendering the component.node_package/src/serverRenderReactComponent.ts (2)
170-175: LGTM!The
stringToStreamfunction correctly converts a string to a stream usingPassThrough.
177-220: LGTM!The
streamServerRenderedReactComponentfunction correctly implements the streaming of rendered React components usingrenderToPipeableStream.
| export const streamServerRenderedReactComponent = (options: RenderParams) => { | ||
| const { name, domNodeId, trace, props, railsContext, throwJsErrors } = options; | ||
|
|
||
| let renderResult: null | PassThrough = null; | ||
|
|
||
| try { | ||
| const componentObj = ComponentRegistry.get(name); | ||
| if (componentObj.isRenderer) { | ||
| throw new Error(`\ | ||
| Detected a renderer while server rendering component '${name}'. \ | ||
| See https://github.com/shakacode/react_on_rails#renderer-functions`); | ||
| } | ||
|
|
||
| const reactRenderingResult = createReactOutput({ | ||
| componentObj, | ||
| domNodeId, | ||
| trace, | ||
| props, | ||
| railsContext, | ||
| }); | ||
|
|
||
| if (isServerRenderHash(reactRenderingResult) || isPromise(reactRenderingResult)) { | ||
| throw new Error('Server rendering of streams is not supported for server render hashes or promises.'); | ||
| } | ||
|
|
||
| renderResult = new PassThrough(); | ||
| ReactDOMServer.renderToPipeableStream(reactRenderingResult as ReactElement).pipe(renderResult); | ||
|
|
||
| // TODO: Add console replay script to the stream | ||
| // Ensure to avoid console messages leaking between different components rendering | ||
| } catch (e: any) { | ||
| if (throwJsErrors) { | ||
| throw e; | ||
| } | ||
|
|
||
| renderResult = stringToStream(handleError({ | ||
| e, | ||
| name, | ||
| serverSide: true, | ||
| })); | ||
| } | ||
|
|
||
| return renderResult; | ||
| }; |
There was a problem hiding this comment.
Enhance error handling with specific error messages.
Consider adding more specific error handling or logging for different types of errors, especially in streaming contexts.
// Example: Add more specific error handling
if (e instanceof SpecificError) {
console.error(`Specific error occurred: ${e.message}`);
} else {
console.error(`General error in streaming: ${e}`);
}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
lib/react_on_rails/helper.rb (1)
104-124: Issues Found:The function
stream_react_component_asyncis defined but not invoked anywhere in the codebase. Additionally, there is no initialization found for@rorp_rendering_fibers. This could lead to runtime errors and incomplete functionality.
- Ensure that
stream_react_component_asyncis called appropriately where needed.- Verify that
@rorp_rendering_fibersis properly initialized before it is used.Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
stream_react_component_asyncare correctly implemented and@rorp_rendering_fibersis properly initialized.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `stream_react_component_async` in the codebase. # Test: Search for the function usage. Expect: Correct implementation of the function calls. rg --type ruby -A 5 $'stream_react_component_async'Length of output: 470
Script:
#!/bin/bash # Description: Verify the usage and initialization of `stream_react_component_async` and `@rorp_rendering_fibers` in the codebase. # Search for all invocations of `stream_react_component_async` in the codebase. rg --type ruby 'stream_react_component_async' # Search for the initialization of `@rorp_rendering_fibers` in the codebase. rg --type ruby '@rorp_rendering_fibers\s*='Length of output: 187
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- lib/react_on_rails/helper.rb (4 hunks)
- spec/dummy/config/webpack/alias.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- spec/dummy/config/webpack/alias.js
Additional comments not posted (2)
lib/react_on_rails/helper.rb (2)
396-420: LGTM! But handle console logs.The
build_react_component_result_for_server_streamed_contentfunction correctly handles the first chunk of the stream and ensures the client can hydrate the app early. The function also correctly marks the content ashtml_safe.However, the TODO comment indicates that handling console logs is pending.
Do you want me to help with handling console logs or open a GitHub issue to track this task?
94-102: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
stream_react_componentare correctly implemented.Verification successful
The search results indicate that
stream_react_componentis used within thestream_react_component_asyncmethod in the same file. To ensure thorough verification, we need to check if there are any other usages ofstream_react_componentin the entire codebase.
Verified: No issues found with the usage of
stream_react_component.The function
stream_react_componentis only used within thestream_react_component_asyncmethod in the same file, and there are no other usages in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `stream_react_component` in the codebase. # Test: Search for the function usage. Expect: Correct implementation of the function calls. rg --type ruby -A 5 $'stream_react_component'Length of output: 1066
Script:
#!/bin/bash # Description: Verify all function calls to `stream_react_component` in the entire codebase. # Test: Search for the function usage in the entire codebase. Expect: Correct implementation of the function calls. rg 'stream_react_component'Length of output: 307
AbanoubGhadban
force-pushed
the
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
branch
from
9ed4ce3 to
0c9955a
Compare
|
|
||
| document.body.innerHTML = '<div id="root"></div>'; | ||
| // eslint-disable-next-line no-underscore-dangle | ||
| const actual = ReactOnRails.render('R1', {}, 'root')._reactInternals.type; |
There was a problem hiding this comment.
When we use a newer react version that supports the Root API, it returns the ReactDomRoot object, which doesn't have the _reactInternals property.
The |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- lib/react_on_rails/helper.rb (5 hunks)
- node_package/src/serverRenderReactComponent.ts (2 hunks)
- spec/dummy/config/webpack/commonWebpackConfig.js (1 hunks)
Additional comments not posted (5)
spec/dummy/config/webpack/commonWebpackConfig.js (1)
44-44: LGTM! The change enhances compatibility.The addition of
process: 'process/browser'in theProvidePluginensures that theprocessobject is available in the browser environment, improving compatibility with libraries that rely on Node.js globals.node_package/src/serverRenderReactComponent.ts (2)
170-175: LGTM! The function correctly converts a string to a stream.The
stringToStreamfunction usesPassThroughto create a stream from a string, which is a correct and efficient approach.
177-220: LGTM! But verify the error handling mechanism.The
streamServerRenderedReactComponentfunction correctly handles server-side rendering through streaming usingrenderToPipeableStream. The error handling mechanism seems robust, but ensure that all possible errors are correctly managed.Verification successful
LGTM! The error handling mechanism in
streamServerRenderedReactComponentis robust.The function correctly handles errors by catching exceptions, checking the
throwJsErrorsflag, and either rethrowing the error or converting it to a stream usinghandleError. This approach is consistent with the rest of the codebase.
node_package/src/serverRenderReactComponent.ts: Lines 177-220Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling mechanism in `streamServerRenderedReactComponent`. # Test: Search for all error handling code. Expect: Proper error handling and logging. rg --type typescript 'catch' -A 5Length of output: 68
Script:
#!/bin/bash # Description: Verify the error handling mechanism in `streamServerRenderedReactComponent`. # Test: Search for all error handling code. Expect: Proper error handling and logging. rg --type ts 'catch' -A 5Length of output: 3753
lib/react_on_rails/helper.rb (2)
355-363: LGTM! The method correctly handles the internal streaming logic.The
stream_react_component_internalmethod merges options with astream?flag and correctly invokes an internal method to generate the component's rendered HTML.
94-114: LGTM! But verify the error handling mechanism.The
stream_react_componentmethod correctly uses Ruby'sFiberto manage asynchronous streaming of React components. The error handling mechanism seems robust, but ensure that all possible errors are correctly managed.Verification successful
Error Handling Verified
The
stream_react_componentmethod's error handling mechanism is robust and consistent with the rest of the codebase. The use ofraisefor error handling is appropriately implemented.
lib/react_on_rails/helper.rblines 94-114: Error handling is verified.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling mechanism in `stream_react_component`. # Test: Search for all error handling code. Expect: Proper error handling and logging. rg --type ruby 'raise' -A 5Length of output: 65682
| def build_react_component_result_for_server_streamed_content( | ||
| rendered_html_stream: required("rendered_html_stream"), | ||
| component_specification_tag: required("component_specification_tag"), | ||
| render_options: required("render_options") | ||
| ) | ||
| content_tag_options_html_tag = render_options.html_options[:tag] || "div" | ||
| # The component_specification_tag is appended to the first chunk | ||
| # We need to pass it early with the first chunk because it's needed in hydration | ||
| # We need to make sure that client can hydrate the app early even before all components are streamed | ||
| is_first_chunk = true | ||
| rendered_html_stream = rendered_html_stream.transform do |chunk| | ||
| if is_first_chunk | ||
| is_first_chunk = false | ||
| html_content = <<-HTML | ||
| #{rails_context_if_not_already_rendered} | ||
| #{component_specification_tag} | ||
| <#{content_tag_options_html_tag} id="#{render_options.dom_id}">#{chunk}</#{content_tag_options_html_tag}> | ||
| HTML | ||
| html_content.strip | ||
| end | ||
| chunk | ||
| end | ||
|
|
||
| rendered_html_stream.transform(&:html_safe) | ||
| # TODO: handle console logs | ||
| end |
There was a problem hiding this comment.
LGTM! But handle the TODO comment.
The build_react_component_result_for_server_streamed_content method correctly manages the specifics of formatting the streamed content. However, the TODO comment indicates that handling console logs is pending.
Do you want me to help with handling console logs or open a GitHub issue to track this task?
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)
- lib/react_on_rails/helper.rb (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails/helper.rb
AbanoubGhadban
requested review from
ahangarha,
alexeyr-ci and
justin808
and removed request for
ahangarha and
justin808
3 participants
Summary
This PR introduces the new features introduced in React 18 which is supporting Async Components and streaming components. It's done by replacing renderToString function with renderToPipeableStream.
https://www.loom.com/share/db55036514b746a7b283e6f296134168?sid=2ce8205d-6956-4e9a-97bc-ee55004f3216
The Dummy App
To open the newly introduced page in the dummy app, you need to clone both React on Rails and React on Rails Pro on your local machine and put them in the same directory
execute the following commands
Then, open the newly introduced
stream_async_componentsin the dummy app.More resources
https://react.dev/reference/react-dom/server/renderToPipeableStream
reactwg/react-18#37
Sequence Diagrams
sequenceDiagram participant Browser participant RailsServer participant ReactOnRails (Ruby) participant ReactOnRailsPro (JS) participant ReactDOMServer Browser->>RailsServer: Request page with React component RailsServer->>ReactOnRails (Ruby): Call `stream_react_component` ReactOnRails (Ruby)->>ReactOnRailsPro (JS): Trigger `streamServerRenderedReactComponent` ReactOnRails (JS)->>ReactDOMServer: Call `renderToPipeableStream` ReactDOMServer-->>ReactOnRailsPro (JS): PipeableStream ReactOnRails (JS)-->>ReactOnRails (Ruby): Stream content ReactOnRails (Ruby)-->>RailsServer: Streamed React component content RailsServer-->>Browser: Serve content Note right of Browser: Displays page with streamed React componentSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Updates
This change is