Separate streamServerRenderedReactComponent from ReactOnRails #1680
Conversation
WalkthroughThis pull request introduces changes to the React on Rails library's server rendering capabilities, specifically removing streaming functionality. The modifications involve deleting the Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)node_package/src/ReactOnRails.ts(1 hunks)node_package/src/serverRenderReactComponent.ts(1 hunks)node_package/src/serverRenderUtils.ts(1 hunks)node_package/src/streamServerRenderedReactComponent.ts(1 hunks)node_package/src/types/index.ts(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(1 hunks)spec/dummy/config/webpack/alias.js(0 hunks)spec/dummy/config/webpack/webpackConfig.js(0 hunks)
💤 Files with no reviewable changes (2)
- spec/dummy/config/webpack/alias.js
- spec/dummy/config/webpack/webpackConfig.js
✅ Files skipped from review due to trivial changes (1)
- node_package/tests/streamServerRenderedReactComponent.test.jsx
👮 Files not reviewed due to content moderation or server errors (3)
- node_package/src/types/index.ts
- node_package/src/ReactOnRails.ts
- CHANGELOG.md
🔇 Additional comments (7)
node_package/src/streamServerRenderedReactComponent.ts (2)
58-105: Stream rendering function implemented correctlyThe
streamRenderReactComponentfunction effectively handles the streaming of the React component usingReactDOMServer.renderToPipeableStream. Error handling and stream management are properly managed through callbacks likeonShellError,onShellReady, andonError.
108-138: Component retrieval and error handling are well-implementedThe
streamServerRenderedReactComponentfunction correctly retrieves and validates the component from the registry, handles rendering results, and manages errors appropriately. The checks for unsupported render results ensure robustness.node_package/src/serverRenderReactComponent.ts (2)
9-9: Improved modularity by importing utilitiesImporting
createResultObject,convertToError, andvalidateComponentfromserverRenderUtilsenhances code modularity and maintainability by separating concerns and reusing common utility functions.
Line range hint
114-164: Server rendering logic is clear and robustThe
serverRenderReactComponentInternalfunction clearly processes the rendering result, handles promises for asynchronous rendering, and properly manages errors throughhandleRenderingError. Resetting the console history ensures consistent logging behavior.node_package/src/serverRenderUtils.ts (3)
4-12: Result object creation handles errors effectivelyThe
createResultObjectfunction correctly constructs theRenderResult, including conditional properties likeisShellReady. This ensures that the rendering state is accurately represented in the final result.
14-16: Error conversion is appropriateThe
convertToErrorfunction guarantees that any thrown exception is standardized as anErrorobject, which simplifies error handling throughout the codebase.
18-22: Component validation enhances robustnessThe
validateComponentfunction ensures that renderers are not used during server rendering by throwing an error ifisRendereris true. This prevents unintended usage and potential runtime errors.
| @@ -0,0 +1,140 @@ | |||
| import ReactDOMServer, { type PipeableStream } from 'react-dom/server'; | |||
| import { PassThrough, Readable } from 'stream'; | |||
There was a problem hiding this comment.
Use Transform instead of PassThrough for custom transform streams
When implementing a custom transform function, you should use the Transform stream instead of PassThrough. The PassThrough stream is designed to pass data through without modification and does not support a custom transform method.
Apply this diff to fix the issue:
-import { PassThrough, Readable } from 'stream';
+import { Transform, Readable } from 'stream';
...
-const transformStream = new PassThrough({
+const transformStream = new Transform({Also applies to: 24-24
* Add export for ReactOnRails for Node.js which supports streaming * adding default export from ReactOnRails module in ReactOnRails.node
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
node_package/src/ReactOnRails.node.ts (1)
1-7: LGTM! Clean separation of streaming functionality.The implementation correctly separates the streaming functionality into a Node-specific file, which aligns with the PR objective of making
stream-browserifyoptional. This change ensures that streaming capabilities are only available in Node.js environments.This architectural change improves the package by:
- Making streaming optional and Node.js specific
- Removing the need for
stream-browserifyin webpack configs- Maintaining backward compatibility through re-exports
node_package/src/ReactOnRails.ts (1)
254-255: Add documentation for streaming migration.The error message clearly indicates Node.js requirement, but consider adding:
- JSDoc comment explaining where the streaming functionality moved to
- Link to migration guide for users who need streaming support
Add this JSDoc comment above the method:
+ /** + * @deprecated Streaming functionality has been moved to @react-on-rails/node-server-side-rendering + * @see https://github.com/shakacode/react_on_rails/blob/master/docs/migration-guide.md + * @throws {Error} When called in browser environment + */ streamServerRenderedReactComponent() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
node_package/src/ReactOnRails.node.ts(1 hunks)node_package/src/ReactOnRails.ts(2 hunks)node_package/src/types/index.ts(1 hunks)package.json(1 hunks)spec/dummy/package.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: build
🔇 Additional comments (5)
package.json (1)
5-10: Well-structured conditional exports configuration.The package.json correctly uses the "exports" field to provide different entry points for Node.js and browser environments. This is a modern approach that:
- Ensures streaming functionality is only available in Node.js
- Prevents webpack from bundling Node.js specific code
Let's verify that the entry points exist:
✅ Verification successful
Exports configuration is correct, files will be generated during build
The package.json exports field correctly references files that will be generated during the build process:
- Source files exist:
node_package/src/ReactOnRails.tsandReactOnRails.node.ts- TypeScript compiler will generate the JavaScript files in
node_package/lib/during build- Build process is properly configured in package scripts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that both entry points exist echo "Checking for Node.js entry point..." ls node_package/lib/ReactOnRails.node.js || echo "Missing Node.js entry point" echo "Checking for browser entry point..." ls node_package/lib/ReactOnRails.js || echo "Missing browser entry point"Length of output: 537
Script:
#!/bin/bash # Search for ReactOnRails files echo "Searching for ReactOnRails files..." fd ReactOnRails # Search for any .js files in node_package directory echo -e "\nSearching in node_package directory..." fd -e js . node_package || echo "No files found in node_package"Length of output: 744
Script:
#!/bin/bash # Check package.json for build configuration echo "Checking package.json for build scripts..." cat package.json | grep -A 10 '"scripts":' # Check for build configuration files echo -e "\nChecking for build configuration files..." fd -e json -e js "tsconfig|rollup|webpack"Length of output: 975
spec/dummy/package.json (2)
75-76: Review development workflow changes.The changes to the build scripts and linking strategy introduce some concerns:
- Removal of
--watchfrombuild:devmight impact development experience- The new
link-sourcescript requires documentationConsider keeping the watch mode or documenting why it was removed. Also, ensure the linking process is documented in the project's README.
Also applies to: 81-81
43-43: Verify the package linking strategy.The change from
file:tolink:in the dependency declaration might affect how the package is resolved during development.Let's verify the linking setup:
✅ Verification successful
Package linking strategy is correctly implemented
The use of
link:.yalc/react-on-railsin spec/dummy/package.json follows the project's documented development workflow. This is the recommended approach as per:
- CONTRIBUTING.md: Details the yalc setup process
- spec/dummy/README.md: Specifically documents using
yalc link🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if yalc is installed and the link exists command -v yalc >/dev/null 2>&1 || echo "yalc is not installed" test -L .yalc/react-on-rails || echo "react-on-rails symlink is missing"Length of output: 178
Script:
#!/bin/bash # Check for yalc configuration and documentation echo "=== Checking for yalc.lock ===" test -f yalc.lock && cat yalc.lock || echo "No yalc.lock found" echo -e "\n=== Searching for yalc mentions in documentation ===" rg -i "yalc" -g "!{package*.json,yarn.lock,*.lock}" --type-add 'docs:*.{md,txt,rst}' -t docs echo -e "\n=== Checking for other .yalc references ===" rg -l "\.yalc" -g "package*.json"Length of output: 2076
node_package/src/types/index.ts (1)
183-199: Well-structured type definitions for rendering states.The new type definitions provide clear separation between regular rendering and streaming states while maintaining type safety.
Good practices observed:
- Clear separation between regular and stream rendering states
- Proper use of TypeScript's utility types (Omit)
- Comprehensive options interface
node_package/src/ReactOnRails.ts (1)
7-7: LGTM! Import changes align with streaming separation.The removal of stream-related imports supports the PR objective of eliminating the
stream-browserifyrequirement.
| "lint": "cd ../.. && yarn run lint", | ||
| "format": "cd ../.. && yarn start format", | ||
| "test": "yarn run build:test && yarn run lint && rspec", | ||
| "build:test": "rm -rf public/webpack/test && yarn build:rescript && RAILS_ENV=test NODE_ENV=test bin/shakapacker", | ||
| "build:dev": "rm -rf public/webpack/development && yarn build:rescript && RAILS_ENV=development NODE_ENV=development bin/shakapacker --watch", | ||
| "build:dev": "rm -rf public/webpack/development && yarn build:rescript && RAILS_ENV=development NODE_ENV=development bin/shakapacker", |
There was a problem hiding this comment.
@Judahmeek @AbanoubGhadban why was the --watch removed?
There was a problem hiding this comment.
The watching variant is build:dev:watch.
There was a problem hiding this comment.
On the other hand, why do we also have previously identical build:dev:server?
There was a problem hiding this comment.
Copilot:
Areas for Improvement
Error Handling: The convertToError function is a good addition, but ensure that all possible error cases are adequately covered.
Stream Handling: The new streamRenderReactComponent function manages stream transformations well. However, consider edge cases where streams might be prematurely closed or where network issues could interrupt the stream. Robust error handling and cleanup mechanisms are essential.
Code Duplication: Ensure that the separation of utility functions into serverRenderUtils.ts doesn't lead to duplicated logic elsewhere in the codebase. Consolidate shared logic to avoid maintenance headaches.
The code review comment suggesting to use a Transform stream instead of PassThrough for custom transform streams is valid. The PassThrough stream is intended for passing data through without any modification, whereas the Transform stream is designed for implementing custom transform functions.
Analysis:
- Current Usage: The
PassThroughstream is used to implement a custom transform function in thetransformRenderStreamChunksToResultObjectfunction. - Improvement: Replacing
PassThroughwithTransformwill align the implementation with the intended use of theTransformstream for custom data transformations.
Suggested Diff:
-import { PassThrough, Readable } from 'stream';
+import { Transform, Readable } from 'stream';
...
-const transformStream = new PassThrough({
+const transformStream = new Transform({This change will make the implementation more appropriate and idiomatic. You can view the relevant file here.
This should revert the breaking change created in 14.1.0 that was requiring users to add
stream-browserifyto their webpack configurations.Summary
Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
Release Notes for Version 14.1.1
Bug Fixes
New Features
Refactor
Chores
stream-browserifypackage dependencies.