-
-
Notifications
You must be signed in to change notification settings - Fork 633
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
[WIP] Hydrate components immediately after downloading chunks #1656
base: abanoubghadban/pro362-add-support-for-RSC
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
7d6577c
to
de85fb4
Compare
c676342
to
266acae
Compare
eb0edbd
to
07e701e
Compare
f2242c4
to
a761735
Compare
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.
Other small issues on the side, the primary implementation of using a CallbackRegistry looks good.
|
||
result << all_stores.each_with_object(declarations) do |redux_store_data, memo| | ||
result << store_objects.each_with_object(declarations) do |redux_store_data, memo| |
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.
How do we make sure that registered stores that aren't listed in the store_dependencies
don't slip through the cracks?
Should we throw a warning if a store is registered that isn't listed in store_dependencies
?
content = File.read(file_path) | ||
# has "use client" directive. It can be "use client" or 'use client' | ||
first_js_statement_in_code(content).match?(/^["']use client["'](?:;|\s|$)/) | ||
end |
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.
Is all this necessary?
Client & server entrypoints should be detectable by their filenames.
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.
React server and client components differ significantly from the server and client files in React on Rails:
-
React on Rails server and client files:
In this system,packs/*.server.js
files are bundled into the server bundle, while*.client.js
files are bundled into the client bundle. -
React server and client components:
This is a new system introduced by React itself. By default, all components are considered server components and are excluded from the client bundle. To designate a component as a client component (an entry point for the client bundle), the"use client";
directive must be added at the top of the file. This system is part of React’s core functionality, not specific to React on Rails.
This code ensures compatibility between both systems within React on Rails.
if bundle_name == ReactOnRails.configuration.server_bundle_js_file | ||
ReactOnRails::Utils.server_bundle_js_file_path | ||
if bundle_name == ReactOnRails.configuration.react_client_manifest_file | ||
ReactOnRails::Utils.react_client_manifest_file_path |
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.
This doesn't make sense.
Neither the ReactOnRails.configuration.react_client_manifest_file
or the ReactOnRails::Utils.react_client_manifest_file_path
currently exist.
Also, I don't see why you would want to replace the server_bundle_js_file
reference with a react_client_manifest_file
reference when the bundle_js_file_path
below basically does the same thing.
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.
Reverted. This code has been moved to PR #1663, which introduced ReactOnRails.configuration.react_client_manifest_file
.
I did not replace the server_bundle_js_file
reference with the react_client_manifest_file
reference because the original code appears to have been written a long time ago when the server bundle path was handled differently. However, with the current implementation, the following code:
if bundle_name == ReactOnRails.configuration.server_bundle_js_file
ReactOnRails::Utils.server_bundle_js_file_path
else
ReactOnRails::Utils.bundle_js_file_path(bundle_name)
end
is now effectively equivalent to:
ReactOnRails::Utils.bundle_js_file_path(bundle_name)
This is because server_bundle_js_file_path
internally calls bundle_js_file_path(ReactOnRails.configuration.server_bundle_js_file)
. Therefore, the updated code you mentioned enables locating the react-client-manifest
file path rather than simply replacing server_bundle
with react-client-manifest
.
07e701e
to
666dc98
Compare
1f3e921
to
999312d
Compare
666dc98
to
9ea28f4
Compare
999312d
to
aeaeca4
Compare
aeaeca4
to
58fd819
Compare
…nt bundle increase
* stream rsc payload in json objects like streamed react components * make path to rsc bundle and react client manifest configurable * feat: Improve client manifest path handling for dev server - Add `dev_server_url` helper to centralize dev server URL construction - Add `public_output_uri_path` to get relative webpack output path - Add `asset_uri_from_packer` to handle asset URIs consistently - Update `react_client_manifest_file_path` to return dev server URLs when appropriate - Add comprehensive specs for new asset URI handling This change ensures client manifest paths are properly resolved to dev server URLs during development, improving hot-reloading functionality. * fix: normalize RSC URL path by absorbing leading/trailing slashes * specify Shakapacker as top-level module * add tests for RSCClientRoot * Make RSCClientRoot tests run with react 18 * Update webpack asset path configuration for client manifest --------- Co-authored-by: Judah Meek <[email protected]>
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