-
-
Notifications
You must be signed in to change notification settings - Fork 636
[WIP] Add ability to render server components inside client components (add support for react-router) #1736
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
base: abanoubghadban/pro465/use-rsc-payload-to-render-server-components-on-server
Are you sure you want to change the base?
Changes from all commits
9ad2741
d4f2a8a
bf46d3f
3772f45
71629c0
32f8b36
94c8a32
552efd2
0a36833
93a8754
b26074f
67e17c7
69f90b7
25c546f
3b93920
b371f19
577b392
63d86d8
ad10b0c
97f440e
676381e
5ba9634
8172e07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -7,10 +7,12 @@ const config: KnipConfig = { | |||
entry: [ | ||||
'node_package/src/ReactOnRails.node.ts!', | ||||
'node_package/src/ReactOnRailsRSC.ts!', | ||||
'node_package/src/registerServerComponent/client.ts!', | ||||
'node_package/src/registerServerComponent/server.ts!', | ||||
'node_package/src/registerServerComponent/client.tsx!', | ||||
'node_package/src/registerServerComponent/server.tsx!', | ||||
'node_package/src/registerServerComponent/server.rsc.ts!', | ||||
'node_package/src/RSCClientRoot.ts!', | ||||
'node_package/src/wrapServerComponentRenderer/server.tsx!', | ||||
'node_package/src/wrapServerComponentRenderer/server.rsc.tsx!', | ||||
'node_package/src/RSCRoute.ts!', | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's imported in
entry shouldn't be necessary (but doesn't hurt either).
|
||||
'eslint.config.ts', | ||||
], | ||||
project: ['node_package/src/**/*.[jt]s{x,}!', 'node_package/tests/**/*.[jt]s{x,}'], | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -312,8 +312,6 @@ def ensure_webpack_generated_files_exists | |
react_client_manifest_file, | ||
react_server_client_manifest_file | ||
].compact_blank | ||
|
||
self.webpack_generated_files = files | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be fixed in the base PR instead. |
||
end | ||
|
||
def configure_skip_display_none_deprecation | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,9 +15,16 @@ class RenderOptions | |
def initialize(react_component_name: required("react_component_name"), options: required("options")) | ||
@react_component_name = react_component_name.camelize | ||
@options = options | ||
# The render_request_id serves as a unique identifier for each render request. | ||
# We cannot rely solely on dom_id, as it should be unique for each component on the page, | ||
# but the server can render the same page multiple times concurrently for different users. | ||
# Therefore, we need an additional unique identifier that can be used both on the client and server. | ||
# This ID can also be used to associate specific data with a particular rendered component | ||
# on either the server or client. | ||
@render_request_id = self.class.generate_request_id | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why this is necessary now and wasn't before. Or is it needed for RSCs in general and just missed in the previous PRs? If yes, can we easily avoid extra work for non-RSCs, even if there's little of it? |
||
end | ||
|
||
attr_reader :react_component_name | ||
attr_reader :react_component_name, :render_request_id | ||
|
||
def throw_js_errors | ||
options.fetch(:throw_js_errors, false) | ||
|
@@ -139,6 +146,10 @@ def store_dependencies | |
options[:store_dependencies] | ||
end | ||
|
||
def self.generate_request_id | ||
SecureRandom.uuid | ||
end | ||
|
||
private | ||
|
||
attr_reader :options | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -128,6 +128,11 @@ def self.react_server_client_manifest_file_path | |||||
return @react_server_manifest_path if @react_server_manifest_path && !Rails.env.development? | ||||||
|
||||||
asset_name = ReactOnRails.configuration.react_server_client_manifest_file | ||||||
if asset_name.nil? | ||||||
raise ReactOnRails::Error, | ||||||
"react_server_client_manifest_file is nil, ensure to set it in your configuration" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
(or |
||||||
end | ||||||
|
||||||
@react_server_manifest_path = File.join(generated_assets_full_path, asset_name) | ||||||
end | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -83,6 +83,18 @@ class ComponentRenderer { | |||||
const { domNodeId } = this; | ||||||
const props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; | ||||||
const trace = el.getAttribute('data-trace') === 'true'; | ||||||
const renderRequestId = el.getAttribute('data-render-request-id'); | ||||||
|
||||||
if (!renderRequestId) { | ||||||
console.error(`renderRequestId is missing for ${name} in dom node with id: ${domNodeId}`); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are the users supposed to do anything in this case?
Suggested change
|
||||||
} | ||||||
|
||||||
const componentSpecificRailsContext = { | ||||||
...railsContext, | ||||||
componentSpecificMetadata: { | ||||||
renderRequestId: renderRequestId || '', | ||||||
}, | ||||||
}; | ||||||
|
||||||
try { | ||||||
const domNode = document.getElementById(domNodeId); | ||||||
|
@@ -93,7 +105,7 @@ class ComponentRenderer { | |||||
} | ||||||
|
||||||
if ( | ||||||
(await delegateToRenderer(componentObj, props, railsContext, domNodeId, trace)) || | ||||||
(await delegateToRenderer(componentObj, props, componentSpecificRailsContext, domNodeId, trace)) || | ||||||
// @ts-expect-error The state can change while awaiting delegateToRenderer | ||||||
this.state === 'unmounted' | ||||||
) { | ||||||
|
@@ -108,7 +120,7 @@ class ComponentRenderer { | |||||
props, | ||||||
domNodeId, | ||||||
trace, | ||||||
railsContext, | ||||||
railsContext: componentSpecificRailsContext, | ||||||
shouldHydrate, | ||||||
}); | ||||||
|
||||||
|
This file was deleted.
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.
Why remove this comment?