-
-
Notifications
You must be signed in to change notification settings - Fork 638
Don't include 'react-dom/server' in the RSC bundle #1888
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
Changes from all commits
efbca35
efaa96f
224751f
e86b8aa
690ed30
41391e2
2aacb21
fa69233
b39c68a
3412273
16472de
44437ff
cf466b7
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 |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import { Readable } from 'stream'; | ||
| import { ErrorOptions } from 'react-on-rails/types'; | ||
| import handleErrorAsString from 'react-on-rails/handleError'; | ||
|
|
||
| const handleError = (options: ErrorOptions) => { | ||
AbanoubGhadban marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const htmlString = handleErrorAsString(options); | ||
| return Readable.from([htmlString]); | ||
| }; | ||
|
|
||
| export default handleError; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import { ErrorOptions } from 'react-on-rails/types'; | ||
| import { renderToPipeableStream } from 'react-on-rails-rsc/server.node'; | ||
| import generateRenderingErrorMessage from 'react-on-rails/generateRenderingErrorMessage'; | ||
|
|
||
| const handleError = (options: ErrorOptions) => { | ||
| const msg = generateRenderingErrorMessage(options); | ||
| return renderToPipeableStream(new Error(msg), { | ||
| filePathToModuleMetadata: {}, | ||
| moduleLoading: { prefix: '', crossOrigin: null }, | ||
| }); | ||
| }; | ||
|
|
||
| export default handleError; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /** | ||
| * @jest-environment node | ||
| */ | ||
|
|
||
| import { PassThrough } from 'stream'; | ||
| import { buildServerRenderer } from 'react-on-rails-rsc/server.node'; | ||
| import { buildClientRenderer } from 'react-on-rails-rsc/client.node'; | ||
|
|
||
| const emptyManifestObject = { | ||
| filePathToModuleMetadata: {}, | ||
| moduleLoading: { prefix: '', crossOrigin: null }, | ||
| }; | ||
|
|
||
| const { renderToPipeableStream } = buildServerRenderer(emptyManifestObject); | ||
| const { createFromNodeStream } = buildClientRenderer(emptyManifestObject, emptyManifestObject); | ||
|
|
||
| test('renderToPipeableStream can encode objects into RSC stream', async () => { | ||
| const encodedStream = renderToPipeableStream({ | ||
| name: 'Alice', | ||
| age: 22, | ||
| }); | ||
| const readableStream = new PassThrough(); | ||
|
|
||
| encodedStream.pipe(readableStream); | ||
| const decodedObject = await createFromNodeStream(readableStream); | ||
| expect(decodedObject).toMatchObject({ | ||
| name: 'Alice', | ||
| age: 22, | ||
| }); | ||
| }); | ||
|
|
||
| test('renderToPipeableStream can encode Error objects into RSC stream', async () => { | ||
| const encodedStream = renderToPipeableStream(new Error('Fake Error')); | ||
| const readableStream = new PassThrough(); | ||
|
|
||
| encodedStream.pipe(readableStream); | ||
| const decodedObject = await createFromNodeStream(readableStream); | ||
| expect(decodedObject).toBeInstanceOf(Error); | ||
| expect(decodedObject).toEqual( | ||
| expect.objectContaining({ | ||
| message: 'Fake Error', | ||
| }), | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| jest.mock('react-dom/server', () => { | ||
| throw new Error("ReactOnRailsRSC shouldn't import react-dom/server at all"); | ||
| }); | ||
|
|
||
| test('import ReactOnRailsRSC', async () => { | ||
| await expect(import('../src/ReactOnRailsRSC.ts')).resolves.toEqual( | ||
| expect.objectContaining({ | ||
| default: expect.objectContaining({ | ||
| isRSCBundle: true, | ||
| }) as unknown, | ||
| }), | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||||||||||||||||||||||||||||||||||||||
| import { createBaseClientObject, type BaseClientObjectType } from './client.ts'; | ||||||||||||||||||||||||||||||||||||||||||
| import type { BaseFullObjectType, ReactOnRailsFullSpecificFunctions } from './full.ts'; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| export type * from './full.ts'; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| export function createBaseFullObject( | ||||||||||||||||||||||||||||||||||||||||||
| registries: Parameters<typeof createBaseClientObject>[0], | ||||||||||||||||||||||||||||||||||||||||||
| currentObject: BaseClientObjectType | null = null, | ||||||||||||||||||||||||||||||||||||||||||
| ): BaseFullObjectType { | ||||||||||||||||||||||||||||||||||||||||||
| // Get or create client object (with caching logic) | ||||||||||||||||||||||||||||||||||||||||||
| const clientObject = createBaseClientObject(registries, currentObject); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Define SSR-specific functions with proper types | ||||||||||||||||||||||||||||||||||||||||||
| // This object acts as a type-safe specification of what we're adding to the base object | ||||||||||||||||||||||||||||||||||||||||||
| const reactOnRailsFullSpecificFunctions: ReactOnRailsFullSpecificFunctions = { | ||||||||||||||||||||||||||||||||||||||||||
| handleError() { | ||||||||||||||||||||||||||||||||||||||||||
| throw new Error('"handleError" function is not supported in RSC bundle'); | ||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| serverRenderReactComponent() { | ||||||||||||||||||||||||||||||||||||||||||
| throw new Error('"serverRenderReactComponent" function is not supported in RSC bundle'); | ||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+15
to
+23
Contributor
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. Fix type signature mismatch in stub functions. The stub implementations don't match the expected signatures from Apply this diff to correct the function signatures: const reactOnRailsFullSpecificFunctions: ReactOnRailsFullSpecificFunctions = {
- handleError() {
+ handleError(options) {
+ void options; // Mark as used
throw new Error('"handleError" function is not supported in RSC bundle');
},
- serverRenderReactComponent() {
+ serverRenderReactComponent(options) {
+ void options; // Mark as used
throw new Error('"serverRenderReactComponent" function is not supported in RSC bundle');
},
};📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Type assertion is safe here because: | ||||||||||||||||||||||||||||||||||||||||||
| // 1. We start with BaseClientObjectType (from createBaseClientObject) | ||||||||||||||||||||||||||||||||||||||||||
| // 2. We add exactly the methods defined in ReactOnRailsFullSpecificFunctions | ||||||||||||||||||||||||||||||||||||||||||
| // 3. BaseFullObjectType = BaseClientObjectType + ReactOnRailsFullSpecificFunctions | ||||||||||||||||||||||||||||||||||||||||||
| // TypeScript can't track the mutation, but we ensure type safety by explicitly typing | ||||||||||||||||||||||||||||||||||||||||||
| // the functions object above | ||||||||||||||||||||||||||||||||||||||||||
| const fullObject = clientObject as unknown as BaseFullObjectType; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Assign SSR-specific functions to the full object using Object.assign | ||||||||||||||||||||||||||||||||||||||||||
| // This pattern ensures we add exactly what's defined in the type, nothing more, nothing less | ||||||||||||||||||||||||||||||||||||||||||
| Object.assign(fullObject, reactOnRailsFullSpecificFunctions); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| return fullObject; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,3 @@ | ||||||
| import * as React from 'react'; | ||||||
| import { renderToString } from './ReactDOMServer.cts'; | ||||||
| import type { ErrorOptions } from './types/index.ts'; | ||||||
|
|
||||||
| function handleRenderFunctionIssue(options: ErrorOptions): string { | ||||||
|
|
@@ -34,7 +32,7 @@ but the React component '${name}' is not a Render-Function.\n${lastLine}`; | |||||
| return msg; | ||||||
| } | ||||||
|
|
||||||
| const handleError = (options: ErrorOptions): string => { | ||||||
| const generateRenderingErrorMessage = (options: ErrorOptions): string => { | ||||||
| const { e, jsCode, serverSide } = options; | ||||||
|
|
||||||
| console.error('Exception in rendering!'); | ||||||
alexeyr-ci2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
@@ -59,16 +57,10 @@ Message: ${e.message} | |||||
|
|
||||||
| ${e.stack}`; | ||||||
|
|
||||||
| // In RSC (React Server Components) bundles, renderToString is not available. | ||||||
| // Therefore, we return the raw error message as a string instead of converting it to HTML. | ||||||
| if (typeof renderToString === 'function') { | ||||||
| const reactElement = React.createElement('pre', null, msg); | ||||||
| return renderToString(reactElement); | ||||||
| } | ||||||
| return msg; | ||||||
| } | ||||||
|
|
||||||
| return 'undefined'; | ||||||
|
Contributor
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. Return undefined instead of the string 'undefined'. Line 63 returns the string literal Apply this diff: - return 'undefined';
+ return undefined;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| }; | ||||||
|
|
||||||
| export default handleError; | ||||||
| export default generateRenderingErrorMessage; | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import * as React from 'react'; | ||
| import { renderToString } from './ReactDOMServer.cts'; | ||
| import type { ErrorOptions } from './types/index.ts'; | ||
| import generateRenderingErrorMessage from './generateRenderingErrorMessage.ts'; | ||
|
|
||
| const handleError = (options: ErrorOptions): string => { | ||
| const msg = generateRenderingErrorMessage(options); | ||
| const reactElement = React.createElement('pre', null, msg); | ||
| return renderToString(reactElement); | ||
| }; | ||
|
|
||
| export default handleError; |
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.
Address the unresolved past review comment and handle edge cases.
The past review comment identified that the current default breaks RSC tests and causes pipeline failures. Additionally, the current implementation doesn't handle edge cases:
"".split(',')returns[""]instead of an empty arrayApply this diff to address both the default and edge cases:
This ensures:
['react-server']🤖 Prompt for AI Agents