From eba248c390a5e32488536a100e2f7c0e55d43da6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 27 Sep 2021 20:47:56 -0400 Subject: [PATCH] [Fizz/Flight] Remove reentrancy hack (#22446) * Remove reentrant check from Fizz/Flight * Make startFlowing explicit in Flight This is already an explicit call in Fizz. This moves flowing to be explicit. That way we can avoid calling it in start() for web streams and therefore avoid the reentrant call. * Add regression test This test doesn't actually error due to the streams polyfill not behaving like Chrome but rather according to spec. * Update the Web Streams polyfill Not that we need this but just in case there are differences that are fixed. --- package.json | 2 +- .../ReactDOMFizzServerBrowser-test.js | 2 +- .../src/ReactNoopFlightServer.js | 1 + .../src/ReactFlightDOMRelayServer.js | 7 +- .../src/ReactFlightDOMServerBrowser.js | 11 +- .../src/ReactFlightDOMServerNode.js | 3 +- .../src/__tests__/ReactFlightDOM-test.js | 2 +- .../__tests__/ReactFlightDOMBrowser-test.js | 269 +++++++++++++++++- .../src/ReactFlightNativeRelayServer.js | 7 +- packages/react-server/src/ReactFizzServer.js | 7 - .../react-server/src/ReactFlightServer.js | 7 - yarn.lock | 11 +- 12 files changed, 300 insertions(+), 29 deletions(-) diff --git a/package.json b/package.json index 03f5aa57c6f50..258a4a093944d 100644 --- a/package.json +++ b/package.json @@ -35,7 +35,7 @@ "@babel/preset-flow": "^7.10.4", "@babel/preset-react": "^7.10.4", "@babel/traverse": "^7.11.0", - "@mattiasbuelens/web-streams-polyfill": "^0.3.2", + "web-streams-polyfill": "^3.1.1", "abort-controller": "^3.0.0", "art": "0.10.1", "babel-eslint": "^10.0.3", diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js index 546e1f622baaa..224dac20d1af4 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js @@ -10,7 +10,7 @@ 'use strict'; // Polyfills for test environment -global.ReadableStream = require('@mattiasbuelens/web-streams-polyfill/ponyfill/es6').ReadableStream; +global.ReadableStream = require('web-streams-polyfill/ponyfill/es6').ReadableStream; global.TextEncoder = require('util').TextEncoder; global.AbortController = require('abort-controller'); diff --git a/packages/react-noop-renderer/src/ReactNoopFlightServer.js b/packages/react-noop-renderer/src/ReactNoopFlightServer.js index 51a5604bd5554..eed5f2219fbfd 100644 --- a/packages/react-noop-renderer/src/ReactNoopFlightServer.js +++ b/packages/react-noop-renderer/src/ReactNoopFlightServer.js @@ -68,6 +68,7 @@ function render(model: ReactModel, options?: Options): Destination { options ? options.onError : undefined, ); ReactNoopFlightServer.startWork(request); + ReactNoopFlightServer.startFlowing(request); return destination; } diff --git a/packages/react-server-dom-relay/src/ReactFlightDOMRelayServer.js b/packages/react-server-dom-relay/src/ReactFlightDOMRelayServer.js index 56769e4255394..fe2f9c8008c85 100644 --- a/packages/react-server-dom-relay/src/ReactFlightDOMRelayServer.js +++ b/packages/react-server-dom-relay/src/ReactFlightDOMRelayServer.js @@ -13,7 +13,11 @@ import type { Destination, } from './ReactFlightDOMRelayServerHostConfig'; -import {createRequest, startWork} from 'react-server/src/ReactFlightServer'; +import { + createRequest, + startWork, + startFlowing, +} from 'react-server/src/ReactFlightServer'; type Options = { onError?: (error: mixed) => void, @@ -32,6 +36,7 @@ function render( options ? options.onError : undefined, ); startWork(request); + startFlowing(request); } export {render}; diff --git a/packages/react-server-dom-webpack/src/ReactFlightDOMServerBrowser.js b/packages/react-server-dom-webpack/src/ReactFlightDOMServerBrowser.js index accd749a56d54..261cf85c9bdf4 100644 --- a/packages/react-server-dom-webpack/src/ReactFlightDOMServerBrowser.js +++ b/packages/react-server-dom-webpack/src/ReactFlightDOMServerBrowser.js @@ -26,7 +26,7 @@ function renderToReadableStream( options?: Options, ): ReadableStream { let request; - return new ReadableStream({ + const stream = new ReadableStream({ start(controller) { request = createRequest( model, @@ -37,10 +37,17 @@ function renderToReadableStream( startWork(request); }, pull(controller) { - startFlowing(request); + // Pull is called immediately even if the stream is not passed to anything. + // That's buffering too early. We want to start buffering once the stream + // is actually used by something so we can give it the best result possible + // at that point. + if (stream.locked) { + startFlowing(request); + } }, cancel(reason) {}, }); + return stream; } export {renderToReadableStream}; diff --git a/packages/react-server-dom-webpack/src/ReactFlightDOMServerNode.js b/packages/react-server-dom-webpack/src/ReactFlightDOMServerNode.js index dc78c503aaf39..4529abd2b6718 100644 --- a/packages/react-server-dom-webpack/src/ReactFlightDOMServerNode.js +++ b/packages/react-server-dom-webpack/src/ReactFlightDOMServerNode.js @@ -37,8 +37,9 @@ function pipeToNodeWritable( webpackMap, options ? options.onError : undefined, ); - destination.on('drain', createDrainHandler(destination, request)); startWork(request); + startFlowing(request); + destination.on('drain', createDrainHandler(destination, request)); } export {pipeToNodeWritable}; diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js index 12a50fce2187e..60fbac215f73d 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js @@ -10,7 +10,7 @@ 'use strict'; // Polyfills for test environment -global.ReadableStream = require('@mattiasbuelens/web-streams-polyfill/ponyfill/es6').ReadableStream; +global.ReadableStream = require('web-streams-polyfill/ponyfill/es6').ReadableStream; global.TextDecoder = require('util').TextDecoder; // Don't wait before processing work on the server. 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 04710cbdc5cf9..8b82caac6bc74 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js @@ -5,28 +5,56 @@ * LICENSE file in the root directory of this source tree. * * @emails react-core - * @jest-environment node */ 'use strict'; // Polyfills for test environment -global.ReadableStream = require('@mattiasbuelens/web-streams-polyfill/ponyfill/es6').ReadableStream; +global.ReadableStream = require('web-streams-polyfill/ponyfill/es6').ReadableStream; global.TextEncoder = require('util').TextEncoder; global.TextDecoder = require('util').TextDecoder; +let webpackModuleIdx = 0; +let webpackModules = {}; +let webpackMap = {}; +global.__webpack_require__ = function(id) { + return webpackModules[id]; +}; + +let act; let React; +let ReactDOM; let ReactServerDOMWriter; let ReactServerDOMReader; describe('ReactFlightDOMBrowser', () => { beforeEach(() => { jest.resetModules(); + webpackModules = {}; + webpackMap = {}; + act = require('jest-react').act; React = require('react'); + ReactDOM = require('react-dom'); ReactServerDOMWriter = require('react-server-dom-webpack/writer.browser.server'); ReactServerDOMReader = require('react-server-dom-webpack'); }); + function moduleReference(moduleExport) { + const idx = webpackModuleIdx++; + webpackModules[idx] = { + d: moduleExport, + }; + webpackMap['path/' + idx] = { + default: { + id: '' + idx, + chunks: [], + name: 'd', + }, + }; + const MODULE_TAG = Symbol.for('react.module.reference'); + return {$$typeof: MODULE_TAG, filepath: 'path/' + idx, name: 'default'}; + } + async function waitForSuspense(fn) { while (true) { try { @@ -75,4 +103,241 @@ describe('ReactFlightDOMBrowser', () => { }); }); }); + + it('should resolve HTML using W3C streams', async () => { + function Text({children}) { + return {children}; + } + function HTML() { + return ( +
+ hello + world +
+ ); + } + + function App() { + const model = { + html: , + }; + return model; + } + + const stream = ReactServerDOMWriter.renderToReadableStream(); + const response = ReactServerDOMReader.createFromReadableStream(stream); + await waitForSuspense(() => { + const model = response.readRoot(); + expect(model).toEqual({ + html: ( +
+ hello + world +
+ ), + }); + }); + }); + + it('should progressively reveal server components', async () => { + let reportedErrors = []; + const {Suspense} = React; + + // Client Components + + class ErrorBoundary extends React.Component { + state = {hasError: false, error: null}; + static getDerivedStateFromError(error) { + return { + hasError: true, + error, + }; + } + render() { + if (this.state.hasError) { + return this.props.fallback(this.state.error); + } + return this.props.children; + } + } + + function MyErrorBoundary({children}) { + return ( +

{e.message}

}> + {children} + + ); + } + + // Model + function Text({children}) { + return children; + } + + function makeDelayedText() { + let error, _resolve, _reject; + let promise = new Promise((resolve, reject) => { + _resolve = () => { + promise = null; + resolve(); + }; + _reject = e => { + error = e; + promise = null; + reject(e); + }; + }); + function DelayedText({children}, data) { + if (promise) { + throw promise; + } + if (error) { + throw error; + } + return {children}; + } + return [DelayedText, _resolve, _reject]; + } + + const [Friends, resolveFriends] = makeDelayedText(); + const [Name, resolveName] = makeDelayedText(); + const [Posts, resolvePosts] = makeDelayedText(); + const [Photos, resolvePhotos] = makeDelayedText(); + const [Games, , rejectGames] = makeDelayedText(); + + // View + function ProfileDetails({avatar}) { + return ( +
+ :name: + {avatar} +
+ ); + } + function ProfileSidebar({friends}) { + return ( +
+ :photos: + {friends} +
+ ); + } + function ProfilePosts({posts}) { + return
{posts}
; + } + function ProfileGames({games}) { + return
{games}
; + } + + const MyErrorBoundaryClient = moduleReference(MyErrorBoundary); + + function ProfileContent() { + return ( + <> + :avatar:} /> + (loading sidebar)

}> + :friends:} /> + + (loading posts)

}> + :posts:} /> + + + (loading games)

}> + :games:} /> + + + + ); + } + + const model = { + rootContent: , + }; + + function ProfilePage({response}) { + return response.readRoot().rootContent; + } + + const stream = ReactServerDOMWriter.renderToReadableStream( + model, + webpackMap, + { + onError(x) { + reportedErrors.push(x); + }, + }, + ); + const response = ReactServerDOMReader.createFromReadableStream(stream); + + const container = document.createElement('div'); + const root = ReactDOM.createRoot(container); + await act(async () => { + root.render( + (loading)

}> + + , + ); + }); + expect(container.innerHTML).toBe('

(loading)

'); + + // This isn't enough to show anything. + await act(async () => { + resolveFriends(); + }); + expect(container.innerHTML).toBe('

(loading)

'); + + // We can now show the details. Sidebar and posts are still loading. + await act(async () => { + resolveName(); + }); + // Advance time enough to trigger a nested fallback. + jest.advanceTimersByTime(500); + expect(container.innerHTML).toBe( + '
:name::avatar:
' + + '

(loading sidebar)

' + + '

(loading posts)

' + + '

(loading games)

', + ); + + expect(reportedErrors).toEqual([]); + + const theError = new Error('Game over'); + // Let's *fail* loading games. + await act(async () => { + rejectGames(theError); + }); + expect(container.innerHTML).toBe( + '
:name::avatar:
' + + '

(loading sidebar)

' + + '

(loading posts)

' + + '

Game over

', // TODO: should not have message in prod. + ); + + expect(reportedErrors).toEqual([theError]); + reportedErrors = []; + + // We can now show the sidebar. + await act(async () => { + resolvePhotos(); + }); + expect(container.innerHTML).toBe( + '
:name::avatar:
' + + '
:photos::friends:
' + + '

(loading posts)

' + + '

Game over

', // TODO: should not have message in prod. + ); + + // Show everything. + await act(async () => { + resolvePosts(); + }); + expect(container.innerHTML).toBe( + '
:name::avatar:
' + + '
:photos::friends:
' + + '
:posts:
' + + '

Game over

', // TODO: should not have message in prod. + ); + + expect(reportedErrors).toEqual([]); + }); }); diff --git a/packages/react-server-native-relay/src/ReactFlightNativeRelayServer.js b/packages/react-server-native-relay/src/ReactFlightNativeRelayServer.js index dc09a3a80d1c5..cae476b46c107 100644 --- a/packages/react-server-native-relay/src/ReactFlightNativeRelayServer.js +++ b/packages/react-server-native-relay/src/ReactFlightNativeRelayServer.js @@ -13,7 +13,11 @@ import type { Destination, } from './ReactFlightNativeRelayServerHostConfig'; -import {createRequest, startWork} from 'react-server/src/ReactFlightServer'; +import { + createRequest, + startWork, + startFlowing, +} from 'react-server/src/ReactFlightServer'; function render( model: ReactModel, @@ -22,6 +26,7 @@ function render( ): void { const request = createRequest(model, destination, config); startWork(request); + startFlowing(request); } export {render}; diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index ff8764ae0696c..c50ef5335297c 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -1748,13 +1748,7 @@ function flushPartiallyCompletedSegment( } } -let reentrant = false; function flushCompletedQueues(request: Request): void { - if (reentrant) { - return; - } - reentrant = true; - const destination = request.destination; beginWriting(destination); try { @@ -1840,7 +1834,6 @@ function flushCompletedQueues(request: Request): void { } largeBoundaries.splice(0, i); } finally { - reentrant = false; completeWriting(destination); flushBuffered(destination); if ( diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index b7a43254fc447..f608c9d288a49 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -706,12 +706,7 @@ function performWork(request: Request): void { } } -let reentrant = false; function flushCompletedChunks(request: Request): void { - if (reentrant) { - return; - } - reentrant = true; const destination = request.destination; beginWriting(destination); try { @@ -758,7 +753,6 @@ function flushCompletedChunks(request: Request): void { } errorChunks.splice(0, i); } finally { - reentrant = false; completeWriting(destination); } flushBuffered(destination); @@ -769,7 +763,6 @@ function flushCompletedChunks(request: Request): void { } export function startWork(request: Request): void { - request.flowing = true; scheduleWork(() => performWork(request)); } diff --git a/yarn.lock b/yarn.lock index bd2d7d8fb7e4a..9deb96000d4e5 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1853,11 +1853,6 @@ "@types/yargs" "^15.0.0" chalk "^4.0.0" -"@mattiasbuelens/web-streams-polyfill@^0.3.2": - version "0.3.2" - resolved "https://registry.yarnpkg.com/@mattiasbuelens/web-streams-polyfill/-/web-streams-polyfill-0.3.2.tgz#d7d180e769ac38f30c4a8e1dd9bd4412affb7f42" - integrity sha512-ANZvP8lC9IXiaPM3rwM8BGMbFIZbbj0goZT/xP2IA95UIZjEToyHXT/k8G0MmSAnxKRMh5E6oLVE6jmOt5zZ/g== - "@nodelib/fs.scandir@2.1.3": version "2.1.3" resolved "https://registry.yarnpkg.com/@nodelib/fs.scandir/-/fs.scandir-2.1.3.tgz#3a582bdb53804c6ba6d146579c46e52130cf4a3b" @@ -6447,6 +6442,7 @@ eslint-plugin-no-unsanitized@3.1.2: "eslint-plugin-react-internal@link:./scripts/eslint-rules": version "0.0.0" + uid "" eslint-plugin-react@^6.7.1: version "6.10.3" @@ -15951,6 +15947,11 @@ web-ext@^4: yargs "15.3.1" zip-dir "1.0.2" +web-streams-polyfill@^3.1.1: + version "3.1.1" + resolved "https://registry.yarnpkg.com/web-streams-polyfill/-/web-streams-polyfill-3.1.1.tgz#1516f2d4ea8f1bdbfed15eb65cb2df87098c8364" + integrity sha512-Czi3fG883e96T4DLEPRvufrF2ydhOOW1+1a6c3gNjH2aIh50DNFBdfwh2AKoOf1rXvpvavAoA11Qdq9+BKjE0Q== + webidl-conversions@^4.0.2: version "4.0.2" resolved "https://registry.yarnpkg.com/webidl-conversions/-/webidl-conversions-4.0.2.tgz#a855980b1f0b6b359ba1d5d9fb39ae941faa63ad"