From a6924d77b10dcf4108f5ffdfd37078b0864c57d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Wed, 25 Mar 2020 11:47:55 -0700 Subject: [PATCH] Change .model getter to .readRoot method (#18382) Originally the idea was to hide all suspending behind getters or proxies. However, this has some issues with perf on hot code like React elements. It also makes it too easy to accidentally access it the first time in an effect or callback where things aren't allowed to suspend. Making it an explicit method call avoids this issue. All other suspending has moved to explicit lazy blocks (and soon elements). The only thing remaining is the root. We could require the root to be an element or block but that creates an unfortunate indirection unnecessarily. Instead, I expose a readRoot method on the response. Typically we try to avoid virtual dispatch but in this case, it's meant that you build abstractions on top of a Flight response so passing it a round is useful. --- fixtures/flight-browser/index.html | 6 +- fixtures/flight/src/App.js | 2 +- fixtures/flight/src/index.js | 3 +- .../react-client/src/ReactFlightClient.js | 69 +++++++---------- .../src/ReactFlightClientStream.js | 22 +++--- .../src/__tests__/ReactFlight-test.js | 11 ++- .../src/ReactFlightDOMRelayClient.js | 15 ++-- .../ReactFlightDOMRelay-test.internal.js | 2 +- .../src/ReactFlightDOMClient.js | 32 ++++---- .../src/__tests__/ReactFlightDOM-test.js | 77 ++++++++++--------- .../__tests__/ReactFlightDOMBrowser-test.js | 5 +- .../src/ReactNoopFlightClient.js | 13 +--- 12 files changed, 122 insertions(+), 135 deletions(-) diff --git a/fixtures/flight-browser/index.html b/fixtures/flight-browser/index.html index e00e78dd48..5d7501bb4a 100644 --- a/fixtures/flight-browser/index.html +++ b/fixtures/flight-browser/index.html @@ -70,20 +70,20 @@ let blob = await responseToDisplay.blob(); let url = URL.createObjectURL(blob); - let data = ReactFlightDOMClient.readFromFetch( + let data = ReactFlightDOMClient.createFromFetch( fetch(url) ); // The client also supports XHR streaming. // var xhr = new XMLHttpRequest(); // xhr.open('GET', url); - // let data = ReactFlightDOMClient.readFromXHR(xhr); + // let data = ReactFlightDOMClient.createFromXHR(xhr); // xhr.send(); renderResult(data); } function Shell({ data }) { - let model = data.model; + let model = data.readRoot(); return

{model.title}

diff --git a/fixtures/flight/src/App.js b/fixtures/flight/src/App.js index 2b177b61c9..e3a7b558b3 100644 --- a/fixtures/flight/src/App.js +++ b/fixtures/flight/src/App.js @@ -1,7 +1,7 @@ import React, {Suspense} from 'react'; function Content({data}) { - return data.model.content; + return data.readRoot().content; } function App({data}) { diff --git a/fixtures/flight/src/index.js b/fixtures/flight/src/index.js index b46341250c..7b3922e3c0 100644 --- a/fixtures/flight/src/index.js +++ b/fixtures/flight/src/index.js @@ -3,5 +3,6 @@ import ReactDOM from 'react-dom'; import ReactFlightDOMClient from 'react-flight-dom-webpack'; import App from './App'; -let data = ReactFlightDOMClient.readFromFetch(fetch('http://localhost:3001')); +let data = ReactFlightDOMClient.createFromFetch(fetch('http://localhost:3001')); + ReactDOM.render(, document.getElementById('root')); diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index f4e9bfb006..ee6275361e 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -27,10 +27,6 @@ import { REACT_ELEMENT_TYPE, } from 'shared/ReactSymbols'; -export type ReactModelRoot = {| - model: T, -|}; - export type JSONValue = | number | null @@ -65,22 +61,32 @@ type ErroredChunk = {| |}; type Chunk = PendingChunk | ResolvedChunk | ErroredChunk; -export type Response = { +export type Response = { partialRow: string, - modelRoot: ReactModelRoot, + rootChunk: Chunk, chunks: Map>, + readRoot(): T, }; -export function createResponse(): Response { - let modelRoot: ReactModelRoot = ({}: any); +function readRoot(): T { + let response: Response = this; + let rootChunk = response.rootChunk; + if (rootChunk.status === RESOLVED) { + return rootChunk.value; + } else { + throw rootChunk.value; + } +} + +export function createResponse(): Response { let rootChunk: Chunk = createPendingChunk(); - definePendingProperty(modelRoot, 'model', rootChunk); let chunks: Map> = new Map(); chunks.set(0, rootChunk); let response = { partialRow: '', - modelRoot, + rootChunk, chunks: chunks, + readRoot: readRoot, }; return response; } @@ -142,7 +148,10 @@ function resolveChunk(chunk: Chunk, value: T): void { // Report that any missing chunks in the model is now going to throw this // error upon read. Also notify any pending promises. -export function reportGlobalError(response: Response, error: Error): void { +export function reportGlobalError( + response: Response, + error: Error, +): void { response.chunks.forEach(chunk => { // If this chunk was already resolved or errored, it won't // trigger an error but if it wasn't then we need to @@ -164,24 +173,6 @@ function readMaybeChunk(maybeChunk: Chunk | T): T { } } -function definePendingProperty( - object: Object, - key: string, - chunk: Chunk, -): void { - Object.defineProperty(object, key, { - configurable: false, - enumerable: true, - get() { - if (chunk.status === RESOLVED) { - return chunk.value; - } else { - throw chunk.value; - } - }, - }); -} - function createElement(type, key, props): React$Element { const element: any = { // This tag allows us to uniquely identify this as a React Element @@ -272,8 +263,8 @@ function createLazyBlock( return lazyType; } -export function parseModelFromJSON( - response: Response, +export function parseModelFromJSON( + response: Response, targetObj: Object, key: string, value: JSONValue, @@ -317,10 +308,10 @@ export function parseModelFromJSON( return value; } -export function resolveModelChunk( - response: Response, +export function resolveModelChunk( + response: Response, id: number, - model: T, + model: M, ): void { let chunks = response.chunks; let chunk = chunks.get(id); @@ -331,8 +322,8 @@ export function resolveModelChunk( } } -export function resolveErrorChunk( - response: Response, +export function resolveErrorChunk( + response: Response, id: number, message: string, stack: string, @@ -348,14 +339,10 @@ export function resolveErrorChunk( } } -export function close(response: Response): void { +export function close(response: Response): void { // In case there are any remaining unresolved chunks, they won't // be resolved now. So we need to issue an error to those. // Ideally we should be able to early bail out if we kept a // ref count of pending chunks. reportGlobalError(response, new Error('Connection closed.')); } - -export function getModelRoot(response: Response): ReactModelRoot { - return response.modelRoot; -} diff --git a/packages/react-client/src/ReactFlightClientStream.js b/packages/react-client/src/ReactFlightClientStream.js index 27e5eabaa8..b7fc86469f 100644 --- a/packages/react-client/src/ReactFlightClientStream.js +++ b/packages/react-client/src/ReactFlightClientStream.js @@ -25,17 +25,13 @@ import { readFinalStringChunk, } from './ReactFlightClientHostConfig'; -export type ReactModelRoot = {| - model: T, -|}; - -type Response = ResponseBase & { +export type Response = ResponseBase & { fromJSON: (key: string, value: JSONValue) => any, stringDecoder: StringDecoder, }; -export function createResponse(): Response { - let response: Response = (createResponseImpl(): any); +export function createResponse(): Response { + let response: Response = (createResponseImpl(): any); response.fromJSON = function(key: string, value: JSONValue) { return parseModelFromJSON(response, this, key, value); }; @@ -45,7 +41,7 @@ export function createResponse(): Response { return response; } -function processFullRow(response: Response, row: string): void { +function processFullRow(response: Response, row: string): void { if (row === '') { return; } @@ -76,8 +72,8 @@ function processFullRow(response: Response, row: string): void { } } -export function processStringChunk( - response: Response, +export function processStringChunk( + response: Response, chunk: string, offset: number, ): void { @@ -92,8 +88,8 @@ export function processStringChunk( response.partialRow += chunk.substring(offset); } -export function processBinaryChunk( - response: Response, +export function processBinaryChunk( + response: Response, chunk: Uint8Array, ): void { if (!supportsBinaryStreams) { @@ -113,4 +109,4 @@ export function processBinaryChunk( response.partialRow += readPartialStringChunk(stringDecoder, chunk); } -export {reportGlobalError, close, getModelRoot} from './ReactFlightClient'; +export {reportGlobalError, close} from './ReactFlightClient'; diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 446a774f70..458d5a036c 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -57,8 +57,7 @@ describe('ReactFlight', () => { let transport = ReactNoopFlightServer.render({ foo: , }); - let root = ReactNoopFlightClient.read(transport); - let model = root.model; + let model = ReactNoopFlightClient.read(transport); expect(model).toEqual({ foo: { bar: ( @@ -87,10 +86,10 @@ describe('ReactFlight', () => { }; let transport = ReactNoopFlightServer.render(model); - let root = ReactNoopFlightClient.read(transport); act(() => { - let UserClient = root.model.User; + let rootModel = ReactNoopFlightClient.read(transport); + let UserClient = rootModel.User; ReactNoop.render(); }); @@ -114,10 +113,10 @@ describe('ReactFlight', () => { }; let transport = ReactNoopFlightServer.render(model); - let root = ReactNoopFlightClient.read(transport); act(() => { - let UserClient = root.model.User; + let rootModel = ReactNoopFlightClient.read(transport); + let UserClient = rootModel.User; ReactNoop.render(); }); diff --git a/packages/react-flight-dom-relay/src/ReactFlightDOMRelayClient.js b/packages/react-flight-dom-relay/src/ReactFlightDOMRelayClient.js index 47bd68c818..11207ba4f8 100644 --- a/packages/react-flight-dom-relay/src/ReactFlightDOMRelayClient.js +++ b/packages/react-flight-dom-relay/src/ReactFlightDOMRelayClient.js @@ -11,14 +11,13 @@ import type {Response, JSONValue} from 'react-client/src/ReactFlightClient'; import { createResponse, - getModelRoot, parseModelFromJSON, resolveModelChunk, resolveErrorChunk, close, } from 'react-client/src/ReactFlightClient'; -function parseModel(response, targetObj, key, value) { +function parseModel(response: Response, targetObj, key, value) { if (typeof value === 'object' && value !== null) { if (Array.isArray(value)) { for (let i = 0; i < value.length; i++) { @@ -38,14 +37,18 @@ function parseModel(response, targetObj, key, value) { return parseModelFromJSON(response, targetObj, key, value); } -export {createResponse, getModelRoot, close}; +export {createResponse, close}; -export function resolveModel(response: Response, id: number, json: JSONValue) { +export function resolveModel( + response: Response, + id: number, + json: JSONValue, +) { resolveModelChunk(response, id, parseModel(response, {}, '', json)); } -export function resolveError( - response: Response, +export function resolveError( + response: Response, id: number, message: string, stack: string, diff --git a/packages/react-flight-dom-relay/src/__tests__/ReactFlightDOMRelay-test.internal.js b/packages/react-flight-dom-relay/src/__tests__/ReactFlightDOMRelay-test.internal.js index c157e3741e..78bcf5c035 100644 --- a/packages/react-flight-dom-relay/src/__tests__/ReactFlightDOMRelay-test.internal.js +++ b/packages/react-flight-dom-relay/src/__tests__/ReactFlightDOMRelay-test.internal.js @@ -39,8 +39,8 @@ describe('ReactFlightDOMRelay', () => { ); } } - let model = ReactDOMFlightRelayClient.getModelRoot(response).model; ReactDOMFlightRelayClient.close(response); + let model = response.readRoot(); return model; } diff --git a/packages/react-flight-dom-webpack/src/ReactFlightDOMClient.js b/packages/react-flight-dom-webpack/src/ReactFlightDOMClient.js index 38bdec83ba..f7d92f07bf 100644 --- a/packages/react-flight-dom-webpack/src/ReactFlightDOMClient.js +++ b/packages/react-flight-dom-webpack/src/ReactFlightDOMClient.js @@ -7,18 +7,20 @@ * @flow */ -import type {ReactModelRoot} from 'react-client/src/ReactFlightClientStream'; +import type {Response as FlightResponse} from 'react-client/src/ReactFlightClientStream'; import { createResponse, - getModelRoot, reportGlobalError, processStringChunk, processBinaryChunk, close, } from 'react-client/src/ReactFlightClientStream'; -function startReadingFromStream(response, stream: ReadableStream): void { +function startReadingFromStream( + response: FlightResponse, + stream: ReadableStream, +): void { let reader = stream.getReader(); function progress({done, value}) { if (done) { @@ -35,16 +37,18 @@ function startReadingFromStream(response, stream: ReadableStream): void { reader.read().then(progress, error); } -function readFromReadableStream(stream: ReadableStream): ReactModelRoot { - let response = createResponse(); +function createFromReadableStream( + stream: ReadableStream, +): FlightResponse { + let response: FlightResponse = createResponse(); startReadingFromStream(response, stream); - return getModelRoot(response); + return response; } -function readFromFetch( +function createFromFetch( promiseForResponse: Promise, -): ReactModelRoot { - let response = createResponse(); +): FlightResponse { + let response: FlightResponse = createResponse(); promiseForResponse.then( function(r) { startReadingFromStream(response, (r.body: any)); @@ -53,11 +57,11 @@ function readFromFetch( reportGlobalError(response, e); }, ); - return getModelRoot(response); + return response; } -function readFromXHR(request: XMLHttpRequest): ReactModelRoot { - let response = createResponse(); +function createFromXHR(request: XMLHttpRequest): FlightResponse { + let response: FlightResponse = createResponse(); let processedLength = 0; function progress(e: ProgressEvent): void { let chunk = request.responseText; @@ -76,7 +80,7 @@ function readFromXHR(request: XMLHttpRequest): ReactModelRoot { request.addEventListener('error', error); request.addEventListener('abort', error); request.addEventListener('timeout', error); - return getModelRoot(response); + return response; } -export {readFromXHR, readFromFetch, readFromReadableStream}; +export {createFromXHR, createFromFetch, createFromReadableStream}; diff --git a/packages/react-flight-dom-webpack/src/__tests__/ReactFlightDOM-test.js b/packages/react-flight-dom-webpack/src/__tests__/ReactFlightDOM-test.js index 468545277e..9dd5133353 100644 --- a/packages/react-flight-dom-webpack/src/__tests__/ReactFlightDOM-test.js +++ b/packages/react-flight-dom-webpack/src/__tests__/ReactFlightDOM-test.js @@ -119,9 +119,10 @@ describe('ReactFlightDOM', () => { let {writable, readable} = getTestStream(); ReactFlightDOMServer.pipeToNodeWritable(, writable, webpackMap); - let result = ReactFlightDOMClient.readFromReadableStream(readable); + let response = ReactFlightDOMClient.createFromReadableStream(readable); await waitForSuspense(() => { - expect(result.model).toEqual({ + let model = response.readRoot(); + expect(model).toEqual({ html: (
hello @@ -154,13 +155,13 @@ describe('ReactFlightDOM', () => { } // View - function Message({result}) { - return
{result.model.html}
; + function Message({response}) { + return
{response.readRoot().html}
; } - function App({result}) { + function App({response}) { return ( Loading...}> - + ); } @@ -171,12 +172,12 @@ describe('ReactFlightDOM', () => { writable, webpackMap, ); - let result = ReactFlightDOMClient.readFromReadableStream(readable); + let response = ReactFlightDOMClient.createFromReadableStream(readable); let container = document.createElement('div'); let root = ReactDOM.createRoot(container); await act(async () => { - root.render(); + root.render(); }); expect(container.innerHTML).toBe( '
helloworld
', @@ -192,13 +193,13 @@ describe('ReactFlightDOM', () => { } // View - function Message({result}) { - return

{result.model.text}

; + function Message({response}) { + return

{response.readRoot().text}

; } - function App({result}) { + function App({response}) { return ( Loading...}> - + ); } @@ -209,12 +210,12 @@ describe('ReactFlightDOM', () => { writable, webpackMap, ); - let result = ReactFlightDOMClient.readFromReadableStream(readable); + let response = ReactFlightDOMClient.createFromReadableStream(readable); let container = document.createElement('div'); let root = ReactDOM.createRoot(container); await act(async () => { - root.render(); + root.render(); }); expect(container.innerHTML).toBe('

$1

'); }); @@ -228,13 +229,13 @@ describe('ReactFlightDOM', () => { } // View - function Message({result}) { - return

{result.model.text}

; + function Message({response}) { + return

{response.readRoot().text}

; } - function App({result}) { + function App({response}) { return ( Loading...}> - + ); } @@ -245,12 +246,12 @@ describe('ReactFlightDOM', () => { writable, webpackMap, ); - let result = ReactFlightDOMClient.readFromReadableStream(readable); + let response = ReactFlightDOMClient.createFromReadableStream(readable); let container = document.createElement('div'); let root = ReactDOM.createRoot(container); await act(async () => { - root.render(); + root.render(); }); expect(container.innerHTML).toBe('

@div

'); }); @@ -327,42 +328,44 @@ describe('ReactFlightDOM', () => { }; // View - function ProfileDetails({result}) { + function ProfileDetails({response}) { + let model = response.readRoot(); return (
- {result.model.name} - {result.model.more.avatar} + {model.name} + {model.more.avatar}
); } - function ProfileSidebar({result}) { + function ProfileSidebar({response}) { + let model = response.readRoot(); return (
- {result.model.photos} - {result.model.more.friends} + {model.photos} + {model.more.friends}
); } - function ProfilePosts({result}) { - return
{result.model.more.posts}
; + function ProfilePosts({response}) { + return
{response.readRoot().more.posts}
; } - function ProfileGames({result}) { - return
{result.model.more.games}
; + function ProfileGames({response}) { + return
{response.readRoot().more.games}
; } - function ProfilePage({result}) { + function ProfilePage({response}) { return ( <> (loading)

}> - + (loading sidebar)

}> - + (loading posts)

}> - +

{e.message}

}> (loading games)

}> - + @@ -372,12 +375,12 @@ describe('ReactFlightDOM', () => { let {writable, readable} = getTestStream(); ReactFlightDOMServer.pipeToNodeWritable(profileModel, writable, webpackMap); - let result = ReactFlightDOMClient.readFromReadableStream(readable); + let response = ReactFlightDOMClient.createFromReadableStream(readable); let container = document.createElement('div'); let root = ReactDOM.createRoot(container); await act(async () => { - root.render(); + root.render(); }); expect(container.innerHTML).toBe('

(loading)

'); diff --git a/packages/react-flight-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js b/packages/react-flight-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js index dd99f31cdb..d33f965253 100644 --- a/packages/react-flight-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-flight-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -62,9 +62,10 @@ describe('ReactFlightDOMBrowser', () => { } let stream = ReactFlightDOMServer.renderToReadableStream(); - let result = ReactFlightDOMClient.readFromReadableStream(stream); + let response = ReactFlightDOMClient.createFromReadableStream(stream); await waitForSuspense(() => { - expect(result.model).toEqual({ + let model = response.readRoot(); + expect(model).toEqual({ html: (
hello diff --git a/packages/react-noop-renderer/src/ReactNoopFlightClient.js b/packages/react-noop-renderer/src/ReactNoopFlightClient.js index e2f9ad0df2..f92f24577f 100644 --- a/packages/react-noop-renderer/src/ReactNoopFlightClient.js +++ b/packages/react-noop-renderer/src/ReactNoopFlightClient.js @@ -14,20 +14,13 @@ * environment. */ -import type {ReactModelRoot} from 'react-client/flight'; - import {readModule} from 'react-noop-renderer/flight-modules'; import ReactFlightClient from 'react-client/flight'; type Source = Array; -const { - createResponse, - getModelRoot, - processStringChunk, - close, -} = ReactFlightClient({ +const {createResponse, processStringChunk, close} = ReactFlightClient({ supportsBinaryStreams: false, resolveModuleReference(idx: string) { return idx; @@ -38,13 +31,13 @@ const { }, }); -function read(source: Source): ReactModelRoot { +function read(source: Source): T { let response = createResponse(source); for (let i = 0; i < source.length; i++) { processStringChunk(response, source[i], 0); } close(response); - return getModelRoot(response); + return response.readRoot(); } export {read};