From f75182a7f979fd15b875acc1cb3478bba552c53d Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sat, 8 Jun 2024 22:14:55 -0400 Subject: [PATCH 1/9] Refactor initializing chunk handling to attach the blocked chunk at the end --- .../react-client/src/ReactFlightClient.js | 163 ++++++++++-------- 1 file changed, 88 insertions(+), 75 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 086a3ae46b802..554c8a8393b49 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -501,13 +501,17 @@ function resolveModuleChunk( } } -let initializingChunk: ResolvedModelChunk = (null: any); -let initializingChunkBlockedModel: null | {deps: number, value: any} = null; +type InitializationHandler = { + chunk: null | BlockedChunk, + value: any, + deps: number, + errored: boolean, +}; +let initializingHandler: null | InitializationHandler = null; + function initializeModelChunk(chunk: ResolvedModelChunk): void { - const prevChunk = initializingChunk; - const prevBlocked = initializingChunkBlockedModel; - initializingChunk = chunk; - initializingChunkBlockedModel = null; + const prevHandler = initializingHandler; + initializingHandler = null; const resolvedModel = chunk.value; @@ -521,31 +525,33 @@ function initializeModelChunk(chunk: ResolvedModelChunk): void { try { const value: T = parseModel(chunk._response, resolvedModel); - if ( - initializingChunkBlockedModel !== null && - initializingChunkBlockedModel.deps > 0 - ) { - initializingChunkBlockedModel.value = value; - // We discovered new dependencies on modules that are not yet resolved. - // We have to go the BLOCKED state until they're resolved. - const blockedChunk: BlockedChunk = (chunk: any); - blockedChunk.status = BLOCKED; - } else { - const resolveListeners = cyclicChunk.value; - const initializedChunk: InitializedChunk = (chunk: any); - initializedChunk.status = INITIALIZED; - initializedChunk.value = value; - if (resolveListeners !== null) { - wakeChunk(resolveListeners, value); + if (initializingHandler !== null) { + if (initializingHandler.errored) { + throw initializingHandler.value; + } + if (initializingHandler.deps > 0) { + // We discovered new dependencies on modules that are not yet resolved. + // We have to go the BLOCKED state until they're resolved. + const blockedChunk: BlockedChunk = (chunk: any); + blockedChunk.status = BLOCKED; + initializingHandler.value = value; + initializingHandler.chunk = blockedChunk; + return; } } + const resolveListeners = cyclicChunk.value; + const initializedChunk: InitializedChunk = (chunk: any); + initializedChunk.status = INITIALIZED; + initializedChunk.value = value; + if (resolveListeners !== null) { + wakeChunk(resolveListeners, value); + } } catch (error) { const erroredChunk: ErroredChunk = (chunk: any); erroredChunk.status = ERRORED; erroredChunk.reason = error; } finally { - initializingChunk = prevChunk; - initializingChunkBlockedModel = prevBlocked; + initializingHandler = prevHandler; } } @@ -725,9 +731,10 @@ function createElement( } // TODO: We should be freezing the element but currently, we might write into // _debugInfo later. We could move it into _store which remains mutable. - if (initializingChunkBlockedModel !== null) { - const freeze = Object.freeze.bind(Object, element.props); - initializingChunk.then(freeze, freeze); + if (initializingHandler !== null) { + // TODO: Restore + // const freeze = Object.freeze.bind(Object, element.props); + // initializingChunk.then(freeze, freeze); } else { Object.freeze(element.props); } @@ -762,57 +769,76 @@ function getChunk(response: Response, id: number): SomeChunk { return chunk; } -function createModelResolver( - chunk: SomeChunk, +function waitForReference( + referencedChunk: PendingChunk | BlockedChunk | CyclicChunk, parentObject: Object, key: string, - cyclic: boolean, response: Response, map: (response: Response, model: any) => T, path: Array, -): (value: any) => void { - let blocked; - if (initializingChunkBlockedModel) { - blocked = initializingChunkBlockedModel; +): T { + const cyclic = referencedChunk.status === CYCLIC; + let handler: InitializationHandler; + if (initializingHandler) { + handler = initializingHandler; if (!cyclic) { - blocked.deps++; + handler.deps++; } } else { - blocked = initializingChunkBlockedModel = { + handler = initializingHandler = { + chunk: null, + value: null, deps: cyclic ? 0 : 1, - value: (null: any), + errored: false, }; } - return value => { - for (let i = 1; i < path.length; i++) { - value = value[path[i]]; - } - parentObject[key] = map(response, value); - // If this is the root object for a model reference, where `blocked.value` - // is a stale `null`, the resolved value can be used directly. - if (key === '' && blocked.value === null) { - blocked.value = parentObject[key]; - } + referencedChunk.then( + (value: any) => { + for (let i = 1; i < path.length; i++) { + value = value[path[i]]; + } + parentObject[key] = map(response, value); + + // If this is the root object for a model reference, where `handler.value` + // is a stale `null`, the resolved value can be used directly. + if (key === '' && handler.value === null) { + handler.value = parentObject[key]; + } - blocked.deps--; - if (blocked.deps === 0) { - if (chunk.status !== BLOCKED) { + if (cyclic || --handler.deps === 0) { + const chunk = handler.chunk; + if (chunk === null || chunk.status !== BLOCKED) { + return; + } + const resolveListeners = chunk.value; + const initializedChunk: InitializedChunk = (chunk: any); + initializedChunk.status = INITIALIZED; + initializedChunk.value = handler.value; + if (resolveListeners !== null) { + wakeChunk(resolveListeners, handler.value); + } + } + }, + error => { + if (handler.errored) { + // We've already errored. We could instead build up an AggregateError + // but if there are multiple errors we just take the first one like + // Promise.all. return; } - const resolveListeners = chunk.value; - const initializedChunk: InitializedChunk = (chunk: any); - initializedChunk.status = INITIALIZED; - initializedChunk.value = blocked.value; - if (resolveListeners !== null) { - wakeChunk(resolveListeners, blocked.value); + handler.errored = true; + handler.value = error; + const chunk = handler.chunk; + if (chunk === null || chunk.status !== BLOCKED) { + return; } - } - }; -} + triggerErrorOnChunk(chunk, error); + }, + ); -function createModelReject(chunk: SomeChunk): (error: mixed) => void { - return (error: mixed) => triggerErrorOnChunk(chunk, error); + // Return a place holder value for now. + return (null: any); } function createServerReferenceProxy, T>( @@ -899,20 +925,7 @@ function getOutlinedModel( case PENDING: case BLOCKED: case CYCLIC: - const parentChunk = initializingChunk; - chunk.then( - createModelResolver( - parentChunk, - parentObject, - key, - chunk.status === CYCLIC, - response, - map, - path, - ), - createModelReject(parentChunk), - ); - return (null: any); + return waitForReference(chunk, parentObject, key, response, map, path); default: throw chunk.reason; } From 63de670f7bb74edd5a382f8654a56945d20312f0 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 9 Jun 2024 15:28:04 -0400 Subject: [PATCH 2/9] Handle blocked or errored handling at a React Element boundary While JSON.parse doesn't let us inspect the begin/complete phase to push/po we can rely on the REACT_ELEMENT_TYPE marker being a leaf that's in the beginning of the element as the begin an the element itself is the complete phase. This lets us more deeply handle suspense or errors in case there's an Error or Suspense boundary in the current model. The tricky bit is to keep in mind cycles and deep references that might now be broken apart. --- .../react-client/src/ReactFlightClient.js | 187 ++++++++++++------ .../__tests__/ReactFlightDOMBrowser-test.js | 47 +++++ 2 files changed, 173 insertions(+), 61 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 554c8a8393b49..3f8d2089d5785 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -72,6 +72,8 @@ import { import getComponentNameFromType from 'shared/getComponentNameFromType'; +import isArray from 'shared/isArray'; + export type {CallServerCallback, EncodeFormActionCallback}; interface FlightStreamController { @@ -502,6 +504,7 @@ function resolveModuleChunk( } type InitializationHandler = { + parent: null | InitializationHandler, chunk: null | BlockedChunk, value: any, deps: number, @@ -515,37 +518,40 @@ function initializeModelChunk(chunk: ResolvedModelChunk): void { const resolvedModel = chunk.value; - // We go to the CYCLIC state until we've fully resolved this. + // We go to the BLOCKED state until we've fully resolved this. // We do this before parsing in case we try to initialize the same chunk // while parsing the model. Such as in a cyclic reference. - const cyclicChunk: CyclicChunk = (chunk: any); - cyclicChunk.status = CYCLIC; + const cyclicChunk: BlockedChunk = (chunk: any); + cyclicChunk.status = BLOCKED; cyclicChunk.value = null; cyclicChunk.reason = null; try { const value: T = parseModel(chunk._response, resolvedModel); + // Invoke any listeners added while resolving this model. I.e. cyclic + // references. This may or may not fully resolve the model depending on + // if they were blocked. + const resolveListeners = cyclicChunk.value; + if (resolveListeners !== null) { + cyclicChunk.value = null; + cyclicChunk.reason = null; + wakeChunk(resolveListeners, value); + } if (initializingHandler !== null) { if (initializingHandler.errored) { throw initializingHandler.value; } if (initializingHandler.deps > 0) { // We discovered new dependencies on modules that are not yet resolved. - // We have to go the BLOCKED state until they're resolved. - const blockedChunk: BlockedChunk = (chunk: any); - blockedChunk.status = BLOCKED; + // We have to keep the BLOCKED state until they're resolved. initializingHandler.value = value; - initializingHandler.chunk = blockedChunk; + initializingHandler.chunk = cyclicChunk; return; } } - const resolveListeners = cyclicChunk.value; const initializedChunk: InitializedChunk = (chunk: any); initializedChunk.status = INITIALIZED; initializedChunk.value = value; - if (resolveListeners !== null) { - wakeChunk(resolveListeners, value); - } } catch (error) { const erroredChunk: ErroredChunk = (chunk: any); erroredChunk.status = ERRORED; @@ -632,7 +638,9 @@ function createElement( owner: null | ReactComponentInfo, // DEV-only stack: null | string, // DEV-only validated: number, // DEV-only -): React$Element { +): + | React$Element + | LazyComponent, SomeChunk>> { let element: any; if (__DEV__ && enableRefAsProp) { // `ref` is non-enumerable in dev @@ -729,16 +737,34 @@ function createElement( value: task, }); } + } + + if (initializingHandler !== null) { + const handler = initializingHandler; + // We pop the stack to the previous outer handler before leaving the Element. + // This is effectively the complete phase. + initializingHandler = handler.parent; + if (handler.errored) { + // TODO: Encode the error as Lazy. + throw handler.value; + } + if (handler.deps > 0) { + // We have blocked references inside this Element but we can turn this into + // a Lazy node referencing this Element to let everything around it proceed. + const blockedChunk: BlockedChunk> = + createBlockedChunk(response); + handler.value = element; + handler.chunk = blockedChunk; + const freeze = Object.freeze.bind(Object, element.props); + blockedChunk.then(freeze, freeze); + return createLazyChunkWrapper(blockedChunk); + } + } else if (__DEV__) { // TODO: We should be freezing the element but currently, we might write into // _debugInfo later. We could move it into _store which remains mutable. - if (initializingHandler !== null) { - // TODO: Restore - // const freeze = Object.freeze.bind(Object, element.props); - // initializingChunk.then(freeze, freeze); - } else { - Object.freeze(element.props); - } + Object.freeze(element.props); } + return element; } @@ -777,65 +803,87 @@ function waitForReference( map: (response: Response, model: any) => T, path: Array, ): T { - const cyclic = referencedChunk.status === CYCLIC; let handler: InitializationHandler; if (initializingHandler) { handler = initializingHandler; - if (!cyclic) { - handler.deps++; - } + handler.deps++; } else { handler = initializingHandler = { + parent: null, chunk: null, value: null, - deps: cyclic ? 0 : 1, + deps: 1, errored: false, }; } - referencedChunk.then( - (value: any) => { - for (let i = 1; i < path.length; i++) { - value = value[path[i]]; - } - parentObject[key] = map(response, value); - - // If this is the root object for a model reference, where `handler.value` - // is a stale `null`, the resolved value can be used directly. - if (key === '' && handler.value === null) { - handler.value = parentObject[key]; - } - - if (cyclic || --handler.deps === 0) { - const chunk = handler.chunk; - if (chunk === null || chunk.status !== BLOCKED) { + function fulfill(value: any): void { + for (let i = 1; i < path.length; i++) { + while (value.$$typeof === REACT_LAZY_TYPE) { + // We never expect to see a Lazy node on this path because we encode those as + // separate models. This must mean that we have inserted an extra lazy node + // e.g. to replace a blocked element. We must instead look for it inside. + const chunk: SomeChunk = value._payload; + if (chunk === handler.chunk) { + // This is a reference to the thing we're currently blocking. We can peak + // inside of it to get the value. + value = handler.value; + continue; + } else if (chunk.status === INITIALIZED) { + value = chunk.value; + continue; + } else { + // If we're not yet initialized we need to skip what we've already drilled + // through and then wait for the next value to become available. + path.splice(0, i - 1); + chunk.then(fulfill, reject); return; } - const resolveListeners = chunk.value; - const initializedChunk: InitializedChunk = (chunk: any); - initializedChunk.status = INITIALIZED; - initializedChunk.value = handler.value; - if (resolveListeners !== null) { - wakeChunk(resolveListeners, handler.value); - } } - }, - error => { - if (handler.errored) { - // We've already errored. We could instead build up an AggregateError - // but if there are multiple errors we just take the first one like - // Promise.all. - return; - } - handler.errored = true; - handler.value = error; + value = value[path[i]]; + } + parentObject[key] = map(response, value); + + // If this is the root object for a model reference, where `handler.value` + // is a stale `null`, the resolved value can be used directly. + if (key === '' && handler.value === null) { + handler.value = parentObject[key]; + } + + handler.deps--; + + if (handler.deps === 0) { const chunk = handler.chunk; if (chunk === null || chunk.status !== BLOCKED) { return; } - triggerErrorOnChunk(chunk, error); - }, - ); + const resolveListeners = chunk.value; + const initializedChunk: InitializedChunk = (chunk: any); + initializedChunk.status = INITIALIZED; + initializedChunk.value = handler.value; + if (resolveListeners !== null) { + wakeChunk(resolveListeners, handler.value); + } + } + } + + function reject(error: mixed): void { + if (handler.errored) { + // We've already errored. We could instead build up an AggregateError + // but if there are multiple errors we just take the first one like + // Promise.all. + return; + } + handler.errored = true; + handler.value = error; + const chunk = handler.chunk; + if (chunk === null || chunk.status !== BLOCKED) { + return; + } + triggerErrorOnChunk(chunk, error); + } + + referencedChunk.then(fulfill, reject); // Return a place holder value for now. return (null: any); @@ -906,7 +954,7 @@ function getOutlinedModel( if ( typeof chunkValue === 'object' && chunkValue !== null && - (Array.isArray(chunkValue) || + (isArray(chunkValue) || typeof chunkValue[ASYNC_ITERATOR] === 'function' || chunkValue.$$typeof === REACT_ELEMENT_TYPE) && !chunkValue._debugInfo @@ -975,6 +1023,23 @@ function parseModelString( if (value[0] === '$') { if (value === '$') { // A very common symbol. + if ( + initializingHandler !== null && + isArray(parentObject) && + key === '0' + ) { + // We we already have an initializing handler and we're abound to enter + // a new element, we need to shadow it because we're now in a new scope. + // This is effectively the "begin" or "push" phase of Element parsing. + // We'll pop later when we parse the array itself. + initializingHandler = { + parent: initializingHandler, + chunk: null, + value: null, + deps: 0, + errored: false, + }; + } return REACT_ELEMENT_TYPE; } switch (value[1]) { diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js index 2d50d6fa427e1..90393e435e91f 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -345,6 +345,53 @@ describe('ReactFlightDOMBrowser', () => { expect(container.innerHTML).toBe('
[[1,2,3],[1,2,3]]
'); }); + it('should resolve deduped objects that are themselves blocked', async () => { + let resolveClientComponentChunk; + + const Client = clientExports( + [4, 5], + '42', + '/test.js', + new Promise(resolve => (resolveClientComponentChunk = resolve)), + ); + + const shared = [1, 2, 3, Client]; + + const stream = await serverAct(() => + ReactServerDOMServer.renderToReadableStream( +
+ + + {shared /* this will serialize first and block nearest element */} + + + {shared /* this will be referenced inside the blocked element */} +
, + webpackMap, + ), + ); + + function ClientRoot({response}) { + return use(response); + } + + const response = ReactServerDOMClient.createFromReadableStream(stream); + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + + await act(() => { + root.render(); + }); + + expect(container.innerHTML).toBe(''); + + await act(() => { + resolveClientComponentChunk(); + }); + + expect(container.innerHTML).toBe('
1234512345
'); + }); + it('should progressively reveal server components', async () => { let reportedErrors = []; From 73cdb999b9b51e8c8916102fe8499995cfe436d0 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 9 Jun 2024 19:55:01 -0400 Subject: [PATCH 3/9] We don't need the cyclic concept anymore It's just a blocked chunk that eagerly calls its listeners. --- packages/react-client/src/ReactFlightClient.js | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 3f8d2089d5785..5dc0c69529ac1 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -103,7 +103,6 @@ type RowParserState = 0 | 1 | 2 | 3 | 4; const PENDING = 'pending'; const BLOCKED = 'blocked'; -const CYCLIC = 'cyclic'; const RESOLVED_MODEL = 'resolved_model'; const RESOLVED_MODULE = 'resolved_module'; const INITIALIZED = 'fulfilled'; @@ -125,14 +124,6 @@ type BlockedChunk = { _debugInfo?: null | ReactDebugInfo, then(resolve: (T) => mixed, reject?: (mixed) => mixed): void, }; -type CyclicChunk = { - status: 'cyclic', - value: null | Array<(T) => mixed>, - reason: null | Array<(mixed) => mixed>, - _response: Response, - _debugInfo?: null | ReactDebugInfo, - then(resolve: (T) => mixed, reject?: (mixed) => mixed): void, -}; type ResolvedModelChunk = { status: 'resolved_model', value: UninitializedModel, @@ -178,7 +169,6 @@ type ErroredChunk = { type SomeChunk = | PendingChunk | BlockedChunk - | CyclicChunk | ResolvedModelChunk | ResolvedModuleChunk | InitializedChunk @@ -220,7 +210,6 @@ Chunk.prototype.then = function ( break; case PENDING: case BLOCKED: - case CYCLIC: if (resolve) { if (chunk.value === null) { chunk.value = ([]: Array<(T) => mixed>); @@ -280,7 +269,6 @@ function readChunk(chunk: SomeChunk): T { return chunk.value; case PENDING: case BLOCKED: - case CYCLIC: // eslint-disable-next-line no-throw-literal throw ((chunk: any): Thenable); default: @@ -329,7 +317,6 @@ function wakeChunkIfInitialized( break; case PENDING: case BLOCKED: - case CYCLIC: if (chunk.value) { for (let i = 0; i < resolveListeners.length; i++) { chunk.value.push(resolveListeners[i]); @@ -796,7 +783,7 @@ function getChunk(response: Response, id: number): SomeChunk { } function waitForReference( - referencedChunk: PendingChunk | BlockedChunk | CyclicChunk, + referencedChunk: PendingChunk | BlockedChunk, parentObject: Object, key: string, response: Response, @@ -972,7 +959,6 @@ function getOutlinedModel( return chunkValue; case PENDING: case BLOCKED: - case CYCLIC: return waitForReference(chunk, parentObject, key, response, map, path); default: throw chunk.reason; From 1b249208e6fc2c0cf9b642324ece0a517498dbf9 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 9 Jun 2024 19:58:55 -0400 Subject: [PATCH 4/9] Fix bug where references deep inside client elements couldn't be deduped. --- .../src/__tests__/ReactFlightDOMEdge-test.js | 15 ++++++------- .../react-server/src/ReactFlightServer.js | 21 ++++++++++++++----- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js index 90597d106b83e..47418276dbb91 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js @@ -224,14 +224,12 @@ describe('ReactFlightDOMEdge', () => { this: {is: 'a large objected'}, with: {many: 'properties in it'}, }; - const props = { - items: new Array(30).fill(obj), - }; + const props = {root:
{new Array(30).fill(obj)}
}; const stream = ReactServerDOMServer.renderToReadableStream(props); const [stream1, stream2] = passThrough(stream).tee(); const serializedContent = await readResult(stream1); - expect(serializedContent.length).toBeLessThan(470); + expect(serializedContent.length).toBeLessThan(1100); const result = await ReactServerDOMClient.createFromReadableStream( stream2, @@ -242,10 +240,13 @@ describe('ReactFlightDOMEdge', () => { }, }, ); + // TODO: Cyclic references currently cause a Lazy wrapper which is not ideal. + const resultElement = result.root._init(result.root._payload); // Should still match the result when parsed - expect(result).toEqual(props); - expect(result.items[5]).toBe(result.items[10]); // two random items are the same instance - // TODO: items[0] is not the same as the others in this case + expect(resultElement).toEqual(props.root); + expect(resultElement.props.children[5]).toBe( + resultElement.props.children[10], + ); // two random items are the same instance }); it('should execute repeated server components only once', async () => { diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 670c11c7c53cf..b8859a898d6fd 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -2117,6 +2117,7 @@ function renderModelDestructive( if (typeof value === 'object') { switch ((value: any).$$typeof) { case REACT_ELEMENT_TYPE: { + let elementReference = null; const writtenObjects = request.writtenObjects; if (task.keyPath !== null || task.implicitSlot) { // If we're in some kind of context we can't reuse the result of this render or @@ -2145,10 +2146,8 @@ function renderModelDestructive( if (parentReference !== undefined) { // If the parent has a reference, we can refer to this object indirectly // through the property name inside that parent. - writtenObjects.set( - value, - parentReference + ':' + parentPropertyName, - ); + elementReference = parentReference + ':' + parentPropertyName; + writtenObjects.set(value, elementReference); } } } @@ -2183,7 +2182,7 @@ function renderModelDestructive( } // Attempt to render the Server Component. - return renderElement( + const newChild = renderElement( request, task, element.type, @@ -2199,6 +2198,18 @@ function renderModelDestructive( : null, __DEV__ && enableOwnerStacks ? element._store.validated : 0, ); + if ( + typeof newChild === 'object' && + newChild !== null && + elementReference !== null + ) { + // If this element renders another object, we can now refer to that object through + // the same location as this element. + if (!writtenObjects.has(newChild)) { + writtenObjects.set(newChild, elementReference); + } + } + return newChild; } case REACT_LAZY_TYPE: { // Reset the task's thenable state before continuing. If there was one, it was From 55e76c4189d85f9cf289d578c7b6f70b3f511243 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 9 Jun 2024 20:36:55 -0400 Subject: [PATCH 5/9] Encode Errors during serialization as Lazy at the Nearest Element We feel comfortable turning any Element into Lazy since it serializes as Node. So if any error happens inside of the deserialization such as if a direct reference errored or a client reference failed to load we can scope it to that element. That way if any Error boundaries were on the stack inside the same model, they can handle the error. This also gives us better debug info for things like serialization errors because they now can get a stack trace pointing to the exact JSX. --- .../react-client/src/ReactFlightClient.js | 76 ++++++++++++++++++- .../src/__tests__/ReactFlight-test.js | 40 ++++++++++ .../react-server/src/ReactFlightServer.js | 21 ++--- 3 files changed, 124 insertions(+), 13 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 5dc0c69529ac1..d316cccfc006c 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -732,8 +732,32 @@ function createElement( // This is effectively the complete phase. initializingHandler = handler.parent; if (handler.errored) { - // TODO: Encode the error as Lazy. - throw handler.value; + // Something errored inside this Element's props. We can turn this Element + // into a Lazy so that we can still render up until that Lazy is rendered. + const erroredChunk: ErroredChunk> = createErrorChunk( + response, + handler.value, + ); + if (__DEV__) { + // Conceptually the error happened inside this Element but right before + // it was rendered. We don't have a client side component to render but + // we can add some DebugInfo to explain that this was conceptually a + // Server side error that errored inside this element. That way any stack + // traces will point to the nearest JSX that errored - e.g. during + // serialization. + const erroredComponent: ReactComponentInfo = { + name: getComponentNameFromType(element.type) || '', + owner: element._owner, + }; + if (enableOwnerStacks) { + // $FlowFixMe[cannot-write] + erroredComponent.stack = element._debugStack; + // $FlowFixMe[cannot-write] + erroredComponent.task = element._debugTask; + } + erroredChunk._debugInfo = [erroredComponent]; + } + return createLazyChunkWrapper(erroredChunk); } if (handler.deps > 0) { // We have blocked references inside this Element but we can turn this into @@ -861,12 +885,43 @@ function waitForReference( // Promise.all. return; } + const blockedValue = handler.value; handler.errored = true; handler.value = error; const chunk = handler.chunk; if (chunk === null || chunk.status !== BLOCKED) { return; } + + if (__DEV__) { + if ( + typeof blockedValue === 'object' && + blockedValue !== null && + blockedValue.$$typeof === REACT_ELEMENT_TYPE + ) { + const element = blockedValue; + // Conceptually the error happened inside this Element but right before + // it was rendered. We don't have a client side component to render but + // we can add some DebugInfo to explain that this was conceptually a + // Server side error that errored inside this element. That way any stack + // traces will point to the nearest JSX that errored - e.g. during + // serialization. + const erroredComponent: ReactComponentInfo = { + name: getComponentNameFromType(element.type) || '', + owner: element._owner, + }; + if (enableOwnerStacks) { + // $FlowFixMe[cannot-write] + erroredComponent.stack = element._debugStack; + // $FlowFixMe[cannot-write] + erroredComponent.task = element._debugTask; + } + const chunkDebugInfo: ReactDebugInfo = + chunk._debugInfo || (chunk._debugInfo = []); + chunkDebugInfo.push(erroredComponent); + } + } + triggerErrorOnChunk(chunk, error); } @@ -961,7 +1016,22 @@ function getOutlinedModel( case BLOCKED: return waitForReference(chunk, parentObject, key, response, map, path); default: - throw chunk.reason; + // This is an error. Instead of erroring directly, we're going to encode this on + // an initialization handler so that we can catch it at the nearest Element. + if (initializingHandler) { + initializingHandler.errored = true; + initializingHandler.value = chunk.reason; + } else { + initializingHandler = { + parent: null, + chunk: null, + value: chunk.reason, + deps: 0, + errored: true, + }; + } + // Placeholder + return (null: any); } } diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 54d66bdac10d0..5507d4cb6b136 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1104,6 +1104,46 @@ describe('ReactFlight', () => { }); }); + it('should handle serialization errors in element inside error boundary', async () => { + const ClientErrorBoundary = clientReference(ErrorBoundary); + + const expectedStack = __DEV__ + ? '\n in div' + '\n in ErrorBoundary (at **)' + '\n in App' + : '\n in ErrorBoundary (at **)'; + + function App() { + return ( + +
+ + ); + } + + const transport = ReactNoopFlightServer.render(, { + onError(x) { + if (__DEV__) { + return 'a dev digest'; + } + if (x instanceof Error) { + return `digest("${x.message}")`; + } else if (Array.isArray(x)) { + return `digest([])`; + } else if (typeof x === 'object' && x !== null) { + return `digest({})`; + } + return `digest(${String(x)})`; + }, + }); + + await act(() => { + startTransition(() => { + ReactNoop.render(ReactNoopFlightClient.read(transport)); + }); + }); + }); + it('should include server components in warning stacks', async () => { function Component() { // Trigger key warning diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index b8859a898d6fd..f5c20ae065b67 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -1060,7 +1060,9 @@ function renderFunctionComponent( owner: owner, }; if (enableOwnerStacks) { - (componentDebugInfo: any).stack = stack; + // $FlowFixMe[prop-missing] + // $FlowFixMe[cannot-write] + componentDebugInfo.stack = stack; } // We outline this model eagerly so that we can refer to by reference as an owner. // If we had a smarter way to dedupe we might not have to do this if there ends up @@ -2076,20 +2078,19 @@ function renderModel( task.keyPath = prevKeyPath; task.implicitSlot = prevImplicitSlot; + // Something errored. We'll still send everything we have up until this point. + request.pendingChunks++; + const errorId = request.nextChunkId++; + const digest = logRecoverableError(request, x); + emitErrorChunk(request, errorId, digest, x); if (wasReactNode) { - // Something errored. We'll still send everything we have up until this point. // We'll replace this element with a lazy reference that throws on the client // once it gets rendered. - request.pendingChunks++; - const errorId = request.nextChunkId++; - const digest = logRecoverableError(request, x); - emitErrorChunk(request, errorId, digest, x); return serializeLazyID(errorId); } - // Something errored but it was not in a React Node. There's no need to serialize - // it by value because it'll just error the whole parent row anyway so we can - // just stop any siblings and error the whole parent row. - throw x; + // If we don't know if it was a React Node we render a direct reference and let + // the client deal with it. + return serializeByValueID(errorId); } } From 89bf5f9f5f9bdf1ea490094c9de4115b0f0bcf51 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 11 Jun 2024 13:28:43 -0400 Subject: [PATCH 6/9] Addressed comments --- packages/react-client/src/ReactFlightClient.js | 12 +++++------- packages/react-reconciler/src/ReactFiberBeginWork.js | 2 +- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index d316cccfc006c..e1d43db1ecffa 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -766,8 +766,10 @@ function createElement( createBlockedChunk(response); handler.value = element; handler.chunk = blockedChunk; - const freeze = Object.freeze.bind(Object, element.props); - blockedChunk.then(freeze, freeze); + if (__DEV__) { + const freeze = Object.freeze.bind(Object, element.props); + blockedChunk.then(freeze, freeze); + } return createLazyChunkWrapper(blockedChunk); } } else if (__DEV__) { @@ -1079,11 +1081,7 @@ function parseModelString( if (value[0] === '$') { if (value === '$') { // A very common symbol. - if ( - initializingHandler !== null && - isArray(parentObject) && - key === '0' - ) { + if (initializingHandler !== null && key === '0') { // We we already have an initializing handler and we're abound to enter // a new element, we need to shadow it because we're now in a new scope. // This is effectively the "begin" or "push" phase of Element parsing. diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 8a03e53f30a6f..5c759d9a52cf1 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -4129,7 +4129,7 @@ function beginWork( } case Throw: { // This represents a Component that threw in the reconciliation phase. - // So we'll rethrow here. This might be + // So we'll rethrow here. This might be a Thenable. throw workInProgress.pendingProps; } } From b452d4d9dee034ab8a4ce4c0e7adb5497727d058 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 11 Jun 2024 21:52:54 +0200 Subject: [PATCH 7/9] More type-safety --- packages/react-server/src/ReactFlightServer.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index f5c20ae065b67..3bbf3cc9634ed 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -1054,13 +1054,12 @@ function renderFunctionComponent( request.pendingChunks++; const componentDebugID = debugID; - componentDebugInfo = { + componentDebugInfo = ({ name: componentName, env: request.environmentName, owner: owner, - }; + }: ReactComponentInfo); if (enableOwnerStacks) { - // $FlowFixMe[prop-missing] // $FlowFixMe[cannot-write] componentDebugInfo.stack = stack; } @@ -2490,15 +2489,16 @@ function renderModelDestructive( ) { // This looks like a ReactComponentInfo. We can't serialize the ConsoleTask object so we // need to omit it before serializing. - const componentDebugInfo = { + const componentDebugInfo: Omit = { name: value.name, env: value.env, - owner: value.owner, + owner: (value: any).owner, }; if (enableOwnerStacks) { - (componentDebugInfo: any).stack = (value: any).stack; + // $FlowFixMe[cannot-write] + componentDebugInfo.stack = (value: any).stack; } - return (componentDebugInfo: any); + return componentDebugInfo; } if (objectName(value) !== 'Object') { From 7d3d3883bbc6d7b713b18aa52653c834bc58f9c6 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 11 Jun 2024 18:19:10 -0400 Subject: [PATCH 8/9] Add Omit helper --- .eslintrc.js | 1 + 1 file changed, 1 insertion(+) diff --git a/.eslintrc.js b/.eslintrc.js index 05a6ecae3ade8..47ea90f62274f 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -571,6 +571,7 @@ module.exports = { TimeoutID: 'readonly', WheelEventHandler: 'readonly', FinalizationRegistry: 'readonly', + Omit: 'readonly', spyOnDev: 'readonly', spyOnDevAndProd: 'readonly', From 977c0788321831d62f9bbf23e9e73620e7e03415 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 11 Jun 2024 18:20:02 -0400 Subject: [PATCH 9/9] Update comment typo --- packages/react-reconciler/src/getComponentNameFromFiber.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/getComponentNameFromFiber.js b/packages/react-reconciler/src/getComponentNameFromFiber.js index fbf5f708e2340..cac247f26912b 100644 --- a/packages/react-reconciler/src/getComponentNameFromFiber.js +++ b/packages/react-reconciler/src/getComponentNameFromFiber.js @@ -164,7 +164,7 @@ export default function getComponentNameFromFiber(fiber: Fiber): string | null { break; case Throw: { if (__DEV__) { - // For an error in child position we use the of the inner most parent component. + // For an error in child position we use the name of the inner most parent component. // Whether a Server Component or the parent Fiber. const debugInfo = fiber._debugInfo; if (debugInfo != null) {