Skip to content

Commit 7e9bc8a

Browse files
AbanoubGhadbanAbanoubGhadban
committed
Refactor generateRSCPayload from global to parameter (#2061)
- Fix stream closed checks for compatibility with different stream implementations - Add tests for concurrent incremental HTML streaming - Fix race condition by using unique bundle paths per test - Add tests for handleRequestClosed on connection close
1 parent 4e1e3ad commit 7e9bc8a

File tree

9 files changed

+135
-56
lines changed

9 files changed

+135
-56
lines changed

‎packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,11 @@
99
}
1010

1111
const runOnOtherBundle = globalThis.runOnOtherBundle;
12-
if (typeof generateRSCPayload !== 'function') {
13-
globalThis.generateRSCPayload = function generateRSCPayload(componentName, props, railsContext) {
14-
const { renderingRequest, rscBundleHash } = railsContext.serverSideRSCPayloadParameters;
15-
const propsString = JSON.stringify(props);
16-
const newRenderingRequest = renderingRequest.replace(/\(\s*\)\s*$/, `('${componentName}', ${propsString})`);
17-
return runOnOtherBundle(rscBundleHash, newRenderingRequest);
18-
}
12+
const generateRSCPayload = function generateRSCPayload(componentName, props, railsContext) {
13+
const { renderingRequest, rscBundleHash } = railsContext.serverSideRSCPayloadParameters;
14+
const propsString = JSON.stringify(props);
15+
const newRenderingRequest = renderingRequest.replace(/\(\s*\)\s*$/, `('${componentName}', ${propsString})`);
16+
return runOnOtherBundle(rscBundleHash, newRenderingRequest);
1917
}
2018

2119
ReactOnRails.clearHydratedStores();
@@ -35,5 +33,6 @@
3533
railsContext: railsContext,
3634
throwJsErrors: false,
3735
renderingReturnsPromises: true,
36+
generateRSCPayload: typeof generateRSCPayload !== 'undefined' ? generateRSCPayload : undefined,
3837
});
3938
})()

‎packages/react-on-rails-pro-node-renderer/tests/httpRequestUtils.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ export const getNextChunkInternal = (
240240

241241
stream.once('data', onData);
242242
stream.once('error', onError);
243-
if (stream.closed) {
243+
if ('closed' in stream && stream.closed) {
244244
onClose();
245245
} else {
246246
stream.once('close', onClose);

‎packages/react-on-rails-pro-node-renderer/tests/incrementalHtmlStreaming.test.ts‎

Lines changed: 56 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import http2 from 'http2';
2-
import * as fs from 'fs';
32
import buildApp from '../src/worker';
4-
import config, { BUNDLE_PATH } from './testingNodeRendererConfigs';
3+
import { createTestConfig } from './testingNodeRendererConfigs';
54
import * as errorReporter from '../src/shared/errorReporter';
65
import {
76
createRenderingRequest,
@@ -13,12 +12,10 @@ import {
1312
} from './httpRequestUtils';
1413
import packageJson from '../src/shared/packageJson';
1514

15+
const { config } = createTestConfig('incrementalHtmlStreaming');
1616
const app = buildApp(config);
1717

1818
beforeAll(async () => {
19-
if (fs.existsSync(BUNDLE_PATH)) {
20-
fs.rmSync(BUNDLE_PATH, { recursive: true, force: true });
21-
}
2219
await app.ready();
2320
await app.listen({ port: 0 });
2421
});
@@ -161,8 +158,7 @@ it('incremental render html', async () => {
161158
close();
162159
});
163160

164-
// TODO: fix the problem of having a global shared `runOnOtherBundle` function
165-
it.skip('raises an error if a specific async prop is not sent', async () => {
161+
it('raises an error if a specific async prop is not sent', async () => {
166162
const { status, body } = await makeRequest();
167163
expect(body).toBe('');
168164
expect(status).toBe(200);
@@ -182,3 +178,56 @@ it.skip('raises an error if a specific async prop is not sent', async () => {
182178
await expect(getNextChunk(request)).rejects.toThrow('Stream Closed');
183179
close();
184180
});
181+
182+
describe('concurrent incremental HTML streaming', () => {
183+
it('handles multiple parallel requests without race conditions', async () => {
184+
await makeRequest();
185+
186+
const numRequests = 5;
187+
const requests = [];
188+
189+
// Start all requests
190+
for (let i = 0; i < numRequests; i += 1) {
191+
const { request, close } = createHttpRequest(RSC_BUNDLE_TIMESTAMP, `concurrent-test-${i}`);
192+
request.write(`${JSON.stringify(createInitialObject())}\n`);
193+
requests.push({ request, close, id: i });
194+
}
195+
196+
// Wait for all to connect and get initial chunks
197+
await Promise.all(requests.map(({ request }) => waitForStatus(request)));
198+
await Promise.all(requests.map(({ request }) => getNextChunk(request)));
199+
200+
// Send update chunks to ALL requests before waiting for any responses
201+
// If sequential: second request wouldn't process until first completes
202+
// If concurrent: all process simultaneously
203+
requests.forEach(({ request, id }) => {
204+
request.write(
205+
`${JSON.stringify({
206+
bundleTimestamp: RSC_BUNDLE_TIMESTAMP,
207+
updateChunk: `
208+
(function(){
209+
var asyncPropsManager = sharedExecutionContext.get("asyncPropsManager");
210+
asyncPropsManager.setProp("books", ["Request-${id}-Book"]);
211+
asyncPropsManager.setProp("researches", ["Request-${id}-Research"]);
212+
})()
213+
`,
214+
})}\n`,
215+
);
216+
request.end();
217+
});
218+
219+
// Now wait for all responses - they should all succeed
220+
const results = await Promise.all(
221+
requests.map(async ({ request, close, id }) => {
222+
const chunk = await getNextChunk(request);
223+
close();
224+
return { id, chunk };
225+
}),
226+
);
227+
228+
results.forEach(({ id, chunk }) => {
229+
expect(chunk).toContain(`Request-${id}-Book`);
230+
expect(chunk).toContain(`Request-${id}-Research`);
231+
});
232+
});
233+
});

‎packages/react-on-rails-pro-node-renderer/tests/incrementalRender.test.ts‎

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,14 @@ describe('incremental render NDJSON endpoint', () => {
6767

6868
const createMockSink = () => {
6969
const sinkAdd = jest.fn();
70+
const handleRequestClosed = jest.fn();
7071

7172
const sink: incremental.IncrementalRenderSink = {
7273
add: sinkAdd,
74+
handleRequestClosed,
7375
};
7476

75-
return { sink, sinkAdd };
77+
return { sink, sinkAdd, handleRequestClosed };
7678
};
7779

7880
const createMockResponse = (data = 'mock response'): ResponseResult => ({
@@ -124,7 +126,7 @@ describe('incremental render NDJSON endpoint', () => {
124126
const createBasicTestSetup = async () => {
125127
await createVmBundle(TEST_NAME);
126128

127-
const { sink, sinkAdd } = createMockSink();
129+
const { sink, sinkAdd, handleRequestClosed } = createMockSink();
128130
const mockResponse = createMockResponse();
129131
const mockResult = createMockResult(sink, mockResponse);
130132

@@ -137,6 +139,7 @@ describe('incremental render NDJSON endpoint', () => {
137139
return {
138140
sink,
139141
sinkAdd,
142+
handleRequestClosed,
140143
mockResponse,
141144
mockResult,
142145
handleSpy,
@@ -158,9 +161,11 @@ describe('incremental render NDJSON endpoint', () => {
158161
});
159162

160163
const sinkAdd = jest.fn();
164+
const handleRequestClosed = jest.fn();
161165

162166
const sink: incremental.IncrementalRenderSink = {
163167
add: sinkAdd,
168+
handleRequestClosed,
164169
};
165170

166171
const mockResponse: ResponseResult = {
@@ -183,6 +188,7 @@ describe('incremental render NDJSON endpoint', () => {
183188
return {
184189
responseStream,
185190
sinkAdd,
191+
handleRequestClosed,
186192
sink,
187193
mockResponse,
188194
mockResult,
@@ -256,7 +262,7 @@ describe('incremental render NDJSON endpoint', () => {
256262
});
257263

258264
test('calls handleIncrementalRenderRequest immediately after first chunk and processes each subsequent chunk immediately', async () => {
259-
const { sinkAdd, handleSpy, SERVER_BUNDLE_TIMESTAMP } = await createBasicTestSetup();
265+
const { sinkAdd, handleRequestClosed, handleSpy, SERVER_BUNDLE_TIMESTAMP } = await createBasicTestSetup();
260266

261267
// Create the HTTP request
262268
const req = createHttpRequest(SERVER_BUNDLE_TIMESTAMP);
@@ -304,6 +310,11 @@ describe('incremental render NDJSON endpoint', () => {
304310
// Final verification: all chunks were processed in the correct order
305311
expect(handleSpy).toHaveBeenCalledTimes(1);
306312
expect(sinkAdd.mock.calls).toEqual([[{ a: 1 }], [{ b: 2 }], [{ c: 3 }]]);
313+
314+
// Verify handleRequestClosed was called when connection closed
315+
await waitFor(() => {
316+
expect(handleRequestClosed).toHaveBeenCalledTimes(1);
317+
});
307318
});
308319

309320
test('returns 410 error when bundle is missing', async () => {
@@ -357,7 +368,7 @@ describe('incremental render NDJSON endpoint', () => {
357368
// Create a bundle for this test
358369
await createVmBundle(TEST_NAME);
359370

360-
const { sink, sinkAdd } = createMockSink();
371+
const { sink, sinkAdd, handleRequestClosed } = createMockSink();
361372

362373
const mockResponse: ResponseResult = createMockResponse();
363374

@@ -416,13 +427,16 @@ describe('incremental render NDJSON endpoint', () => {
416427
await waitFor(() => {
417428
expect(sinkAdd.mock.calls).toEqual([[{ a: 1 }], [{ d: 4 }]]);
418429
});
430+
431+
// Verify handleRequestClosed was called when connection closed
432+
expect(handleRequestClosed).toHaveBeenCalledTimes(1);
419433
});
420434

421435
test('handles empty lines gracefully in the stream', async () => {
422436
// Create a bundle for this test
423437
await createVmBundle(TEST_NAME);
424438

425-
const { sink, sinkAdd } = createMockSink();
439+
const { sink, sinkAdd, handleRequestClosed } = createMockSink();
426440

427441
const mockResponse: ResponseResult = createMockResponse();
428442

@@ -469,6 +483,11 @@ describe('incremental render NDJSON endpoint', () => {
469483
// Verify that only valid JSON objects were processed
470484
expect(handleSpy).toHaveBeenCalledTimes(1);
471485
expect(sinkAdd.mock.calls).toEqual([[{ a: 1 }], [{ b: 2 }], [{ c: 3 }]]);
486+
487+
// Verify handleRequestClosed was called when connection closed
488+
await waitFor(() => {
489+
expect(handleRequestClosed).toHaveBeenCalledTimes(1);
490+
});
472491
});
473492

474493
test('throws error when first chunk processing fails (e.g., authentication)', async () => {
@@ -515,7 +534,8 @@ describe('incremental render NDJSON endpoint', () => {
515534
'Goodbye from stream',
516535
];
517536

518-
const { responseStream, sinkAdd, handleSpy, SERVER_BUNDLE_TIMESTAMP } = await createStreamingTestSetup();
537+
const { responseStream, sinkAdd, handleRequestClosed, handleSpy, SERVER_BUNDLE_TIMESTAMP } =
538+
await createStreamingTestSetup();
519539

520540
// write the response chunks to the stream
521541
let sentChunkIndex = 0;
@@ -586,10 +606,14 @@ describe('incremental render NDJSON endpoint', () => {
586606

587607
// Verify that the mock was called correctly
588608
expect(handleSpy).toHaveBeenCalledTimes(1);
609+
610+
// Verify handleRequestClosed was called when connection closed
611+
expect(handleRequestClosed).toHaveBeenCalledTimes(1);
589612
});
590613

591614
test('echo server - processes each chunk and immediately streams it back', async () => {
592-
const { responseStream, sinkAdd, handleSpy, SERVER_BUNDLE_TIMESTAMP } = await createStreamingTestSetup();
615+
const { responseStream, sinkAdd, handleRequestClosed, handleSpy, SERVER_BUNDLE_TIMESTAMP } =
616+
await createStreamingTestSetup();
593617

594618
// Create the HTTP request
595619
const req = createHttpRequest(SERVER_BUNDLE_TIMESTAMP);
@@ -677,6 +701,9 @@ describe('incremental render NDJSON endpoint', () => {
677701

678702
// Verify that the mock was called correctly
679703
expect(handleSpy).toHaveBeenCalledTimes(1);
704+
705+
// Verify handleRequestClosed was called when connection closed
706+
expect(handleRequestClosed).toHaveBeenCalledTimes(1);
680707
});
681708

682709
describe('incremental render update chunk functionality', () => {

‎packages/react-on-rails-pro-node-renderer/tests/worker.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -894,7 +894,7 @@ describe('worker', () => {
894894
400,
895895
);
896896

897-
expect(res.payload).toContain('INVALID NIL or NULL result for rendering');
897+
expect(res.payload).toContain('Invalid first incremental render request chunk received');
898898
});
899899

900900
test('fails when password is missing', async () => {

‎packages/react-on-rails-pro/src/RSCRequestTracker.ts‎

Lines changed: 14 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,26 +17,10 @@ import {
1717
RSCPayloadStreamInfo,
1818
RSCPayloadCallback,
1919
RailsContextWithServerComponentMetadata,
20+
GenerateRSCPayloadFunction,
2021
} from 'react-on-rails/types';
2122
import { extractErrorMessage } from './utils.ts';
2223

23-
/**
24-
* Global function provided by React on Rails Pro for generating RSC payloads.
25-
*
26-
* This function is injected into the global scope during server-side rendering
27-
* by the RORP rendering request. It handles the actual generation of React Server
28-
* Component payloads on the server side.
29-
*
30-
* @see https://github.com/shakacode/react_on_rails_pro/blob/master/lib/react_on_rails_pro/server_rendering_js_code.rb
31-
*/
32-
declare global {
33-
function generateRSCPayload(
34-
componentName: string,
35-
props: unknown,
36-
railsContext: RailsContextWithServerComponentMetadata,
37-
): Promise<NodeJS.ReadableStream>;
38-
}
39-
4024
/**
4125
* RSC Request Tracker - manages RSC payload generation and tracking for a single request.
4226
*
@@ -52,8 +36,14 @@ class RSCRequestTracker {
5236

5337
private railsContext: RailsContextWithServerComponentMetadata;
5438

55-
constructor(railsContext: RailsContextWithServerComponentMetadata) {
39+
private generateRSCPayload?: GenerateRSCPayloadFunction;
40+
41+
constructor(
42+
railsContext: RailsContextWithServerComponentMetadata,
43+
generateRSCPayload?: GenerateRSCPayloadFunction,
44+
) {
5645
this.railsContext = railsContext;
46+
this.generateRSCPayload = generateRSCPayload;
5747
}
5848

5949
/**
@@ -120,17 +110,17 @@ class RSCRequestTracker {
120110
* @throws Error if generateRSCPayload is not available or fails
121111
*/
122112
async getRSCPayloadStream(componentName: string, props: unknown): Promise<NodeJS.ReadableStream> {
123-
// Validate that the global generateRSCPayload function is available
124-
if (typeof generateRSCPayload !== 'function') {
113+
// Validate that the generateRSCPayload function is available
114+
if (!this.generateRSCPayload) {
125115
throw new Error(
126-
'generateRSCPayload is not defined. Please ensure that you are using at least version 4.0.0 of ' +
127-
'React on Rails Pro and the Node renderer, and that ReactOnRailsPro.configuration.enable_rsc_support ' +
128-
'is set to true.',
116+
'generateRSCPayload function is not available. This could mean: ' +
117+
'(1) ReactOnRailsPro.configuration.enable_rsc_support is not enabled, or ' +
118+
'(2) You are using an incompatible version of React on Rails Pro (requires 4.0.0+).',
129119
);
130120
}
131121

132122
try {
133-
const stream = await generateRSCPayload(componentName, props, this.railsContext);
123+
const stream = await this.generateRSCPayload(componentName, props, this.railsContext);
134124

135125
// Tee stream to allow for multiple consumers:
136126
// 1. stream1 - Used by React's runtime to perform server-side rendering

‎packages/react-on-rails-pro/src/streamingUtils.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,11 +187,19 @@ export const streamServerRenderedComponent = <T, P extends RenderParams>(
187187
renderStrategy: StreamRenderer<T, P>,
188188
handleError: (options: ErrorOptions) => PipeableOrReadableStream,
189189
): T => {
190-
const { name: componentName, domNodeId, trace, props, railsContext, throwJsErrors } = options;
190+
const {
191+
name: componentName,
192+
domNodeId,
193+
trace,
194+
props,
195+
railsContext,
196+
throwJsErrors,
197+
generateRSCPayload,
198+
} = options;
191199

192200
assertRailsContextWithServerComponentMetadata(railsContext);
193201
const postSSRHookTracker = new PostSSRHookTracker();
194-
const rscRequestTracker = new RSCRequestTracker(railsContext);
202+
const rscRequestTracker = new RSCRequestTracker(railsContext, generateRSCPayload);
195203
const streamingTrackers = {
196204
postSSRHookTracker,
197205
rscRequestTracker,

‎packages/react-on-rails/src/types/index.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,11 +216,18 @@ export interface RegisteredComponent {
216216

217217
export type ItemRegistrationCallback<T> = (component: T) => void;
218218

219+
export type GenerateRSCPayloadFunction = (
220+
componentName: string,
221+
props: unknown,
222+
railsContext: RailsContextWithServerComponentMetadata,
223+
) => Promise<NodeJS.ReadableStream>;
224+
219225
interface Params {
220226
props?: Record<string, unknown>;
221227
railsContext?: RailsContext;
222228
domNodeId?: string;
223229
trace?: boolean;
230+
generateRSCPayload?: GenerateRSCPayloadFunction;
224231
}
225232

226233
export interface RenderParams extends Params {

0 commit comments

Comments
 (0)