Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes (f…

…acebook#24030)

* I forgot to call onFatalError

I can't figure out how to write a test for this because it only happens
when there is a bug in React itself which would then be fixed if we found
it.

We're also covered by the protection of ReadableStream which doesn't leak
other errors to us.

* Abort requests if the reader cancels

No need to continue computing at this point.

* Abort requests if node streams get destroyed

This is if the downstream cancels is for example.

* Rename Node APIs for Parity with allReady

The "Complete" terminology is a little misleading because not everything
has been written yet. It's just "Ready" to be written now.

onShellReady
onShellError
onAllReady

* 'close' should be enough

fixtures/ssr/server/render.js

@@ -22,13 +22,13 @@ export default function render(url, res) {
   let didError = false;
   const {pipe, abort} = renderToPipeableStream(<App assets={assets} />, {
     bootstrapScripts: [assets['main.js']],
-    onCompleteShell() {
+    onShellReady() {
       // If something errored before we started streaming, we set the error code appropriately.
       res.statusCode = didError ? 500 : 200;
       res.setHeader('Content-type', 'text/html');
       pipe(res);
     },
-    onErrorShell(x) {
+    onShellError(x) {
       // Something errored before we could complete the shell so we emit an alternative shell.
       res.statusCode = 500;
       res.send('<!doctype><p>Error</p>');

fixtures/ssr2/server/render.js

@@ -49,7 +49,7 @@ module.exports = function render(url, res) {
         res.setHeader('Content-type', 'text/html');
         pipe(res);
       },
-      onErrorShell(x) {
+      onShellError(x) {
         // Something errored before we could complete the shell so we emit an alternative shell.
         res.statusCode = 500;
         res.send('<!doctype><p>Error</p>');

packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js

@@ -914,7 +914,7 @@ describe('ReactDOMFizzServer', () => {
         </Suspense>,
         {
           identifierPrefix: 'A_',
-          onCompleteShell() {
+          onShellReady() {
             writableA.write('<div id="container-A">');
             pipe(writableA);
             writableA.write('</div>');
@@ -933,7 +933,7 @@ describe('ReactDOMFizzServer', () => {
         </Suspense>,
         {
           identifierPrefix: 'B_',
-          onCompleteShell() {
+          onShellReady() {
             writableB.write('<div id="container-B">');
             pipe(writableB);
             writableB.write('</div>');
@@ -1168,7 +1168,7 @@ describe('ReactDOMFizzServer', () => {
 
         {
           namespaceURI: 'http://www.w3.org/2000/svg',
-          onCompleteShell() {
+          onShellReady() {
             writable.write('<svg>');
             pipe(writable);
             writable.write('</svg>');

packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js

@@ -209,4 +209,43 @@ describe('ReactDOMFizzServer', () => {
     const result = await readResult(stream);
     expect(result).toContain('Loading');
   });
+
+  // @gate experimental
+  it('should not continue rendering after the reader cancels', async () => {
+    let hasLoaded = false;
+    let resolve;
+    let isComplete = false;
+    let rendered = false;
+    const promise = new Promise(r => (resolve = r));
+    function Wait() {
+      if (!hasLoaded) {
+        throw promise;
+      }
+      rendered = true;
+      return 'Done';
+    }
+    const stream = await ReactDOMFizzServer.renderToReadableStream(
+      <div>
+        <Suspense fallback={<div>Loading</div>}>
+          <Wait /> />
+        </Suspense>
+      </div>,
+    );
+
+    stream.allReady.then(() => (isComplete = true));
+
+    expect(rendered).toBe(false);
+    expect(isComplete).toBe(false);
+
+    const reader = stream.getReader();
+    reader.cancel();
+
+    hasLoaded = true;
+    resolve();
+
+    await jest.runAllTimers();
+
+    expect(rendered).toBe(false);
+    expect(isComplete).toBe(true);
+  });
 });

packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js

@@ -138,7 +138,7 @@ describe('ReactDOMFizzServer', () => {
       </div>,
 
       {
-        onCompleteAll() {
+        onAllReady() {
           isCompleteCalls++;
         },
       },
@@ -179,7 +179,7 @@ describe('ReactDOMFizzServer', () => {
         onError(x) {
           reportedErrors.push(x);
         },
-        onErrorShell(x) {
+        onShellError(x) {
           reportedShellErrors.push(x);
         },
       },
@@ -213,7 +213,7 @@ describe('ReactDOMFizzServer', () => {
         onError(x) {
           reportedErrors.push(x);
         },
-        onErrorShell(x) {
+        onShellError(x) {
           reportedShellErrors.push(x);
         },
       },
@@ -244,7 +244,7 @@ describe('ReactDOMFizzServer', () => {
         onError(x) {
           reportedErrors.push(x);
         },
-        onErrorShell(x) {
+        onShellError(x) {
           reportedShellErrors.push(x);
         },
       },
@@ -298,7 +298,7 @@ describe('ReactDOMFizzServer', () => {
       </div>,
 
       {
-        onCompleteAll() {
+        onAllReady() {
           isCompleteCalls++;
         },
       },
@@ -333,7 +333,7 @@ describe('ReactDOMFizzServer', () => {
       </div>,
 
       {
-        onCompleteAll() {
+        onAllReady() {
           isCompleteCalls++;
         },
       },
@@ -537,4 +537,49 @@ describe('ReactDOMFizzServer', () => {
     expect(output.result).not.toContain('context never found');
     expect(output.result).toContain('OK');
   });
+
+  // @gate experimental
+  it('should not continue rendering after the writable ends unexpectedly', async () => {
+    let hasLoaded = false;
+    let resolve;
+    let isComplete = false;
+    let rendered = false;
+    const promise = new Promise(r => (resolve = r));
+    function Wait() {
+      if (!hasLoaded) {
+        throw promise;
+      }
+      rendered = true;
+      return 'Done';
+    }
+    const {writable, completed} = getTestWritable();
+    const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
+      <div>
+        <Suspense fallback={<div>Loading</div>}>
+          <Wait />
+        </Suspense>
+      </div>,
+      {
+        onAllReady() {
+          isComplete = true;
+        },
+      },
+    );
+    pipe(writable);
+
+    expect(rendered).toBe(false);
+    expect(isComplete).toBe(false);
+
+    writable.end();
+
+    await jest.runAllTimers();
+
+    hasLoaded = true;
+    resolve();
+
+    await completed;
+
+    expect(rendered).toBe(false);
+    expect(isComplete).toBe(true);
+  });
 });

packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js

@@ -155,7 +155,7 @@ module.exports = function(initModules) {
         new Promise((resolve, reject) => {
           const writable = new DrainWritable();
           const s = ReactDOMServer.renderToPipeableStream(reactElement, {
-            onErrorShell(e) {
+            onShellError(e) {
               reject(e);
             },
           });

packages/react-dom/src/server/ReactDOMFizzServerBrowser.js

@@ -46,25 +46,27 @@ function renderToReadableStream(
 ): Promise<ReactDOMServerReadableStream> {
   return new Promise((resolve, reject) => {
     let onFatalError;
-    let onCompleteAll;
+    let onAllReady;
     const allReady = new Promise((res, rej) => {
-      onCompleteAll = res;
+      onAllReady = res;
       onFatalError = rej;
     });
 
-    function onCompleteShell() {
+    function onShellReady() {
       const stream: ReactDOMServerReadableStream = (new ReadableStream({
         type: 'bytes',
         pull(controller) {
           startFlowing(request, controller);
         },
-        cancel(reason) {},
+        cancel(reason) {
+          abort(request);
+        },
       }): any);
       // TODO: Move to sub-classing ReadableStream.
       stream.allReady = allReady;
       resolve(stream);
     }
-    function onErrorShell(error: mixed) {
+    function onShellError(error: mixed) {
       reject(error);
     }
     const request = createRequest(
@@ -79,9 +81,9 @@ function renderToReadableStream(
       createRootFormatContext(options ? options.namespaceURI : undefined),
       options ? options.progressiveChunkSize : undefined,
       options ? options.onError : undefined,
-      onCompleteAll,
-      onCompleteShell,
-      onErrorShell,
+      onAllReady,
+      onShellReady,
+      onShellError,
       onFatalError,
     );
     if (options && options.signal) {

packages/react-dom/src/server/ReactDOMFizzServerNode.js

@@ -28,6 +28,10 @@ function createDrainHandler(destination, request) {
   return () => startFlowing(request, destination);
 }
 
+function createAbortHandler(request) {
+  return () => abort(request);
+}
+
 type Options = {|
   identifierPrefix?: string,
   namespaceURI?: string,
@@ -36,9 +40,9 @@ type Options = {|
   bootstrapScripts?: Array<string>,
   bootstrapModules?: Array<string>,
   progressiveChunkSize?: number,
-  onCompleteShell?: () => void,
-  onErrorShell?: () => void,
-  onCompleteAll?: () => void,
+  onShellReady?: () => void,
+  onShellError?: () => void,
+  onAllReady?: () => void,
   onError?: (error: mixed) => void,
 |};
 
@@ -62,9 +66,9 @@ function createRequestImpl(children: ReactNodeList, options: void | Options) {
     createRootFormatContext(options ? options.namespaceURI : undefined),
     options ? options.progressiveChunkSize : undefined,
     options ? options.onError : undefined,
-    options ? options.onCompleteAll : undefined,
-    options ? options.onCompleteShell : undefined,
-    options ? options.onErrorShell : undefined,
+    options ? options.onAllReady : undefined,
+    options ? options.onShellReady : undefined,
+    options ? options.onShellError : undefined,
     undefined,
   );
 }
@@ -86,6 +90,7 @@ function renderToPipeableStream(
       hasStartedFlowing = true;
       startFlowing(request, destination);
       destination.on('drain', createDrainHandler(destination, request));
+      destination.on('close', createAbortHandler(request));
       return destination;
     },
     abort() {

packages/react-dom/src/server/ReactDOMLegacyServerBrowser.js

@@ -53,7 +53,7 @@ function renderToStringImpl(
   };
 
   let readyToStream = false;
-  function onCompleteShell() {
+  function onShellReady() {
     readyToStream = true;
   }
   const request = createRequest(
@@ -66,7 +66,7 @@ function renderToStringImpl(
     Infinity,
     onError,
     undefined,
-    onCompleteShell,
+    onShellReady,
     undefined,
     undefined,
   );

packages/react-dom/src/server/ReactDOMLegacyServerNode.js

@@ -68,7 +68,7 @@ function renderToNodeStreamImpl(
   options: void | ServerOptions,
   generateStaticMarkup: boolean,
 ): Readable {
-  function onCompleteAll() {
+  function onAllReady() {
     // We wait until everything has loaded before starting to write.
     // That way we only end up with fully resolved HTML even if we suspend.
     destination.startedFlowing = true;
@@ -81,7 +81,7 @@ function renderToNodeStreamImpl(
     createRootFormatContext(),
     Infinity,
     onError,
-    onCompleteAll,
+    onAllReady,
     undefined,
     undefined,
   );

packages/react-noop-renderer/src/ReactNoopServer.js

@@ -251,8 +251,8 @@ const ReactNoopServer = ReactFizzServer({
 
 type Options = {
   progressiveChunkSize?: number,
-  onCompleteShell?: () => void,
-  onCompleteAll?: () => void,
+  onShellReady?: () => void,
+  onAllReady?: () => void,
   onError?: (error: mixed) => void,
 };
 
@@ -272,8 +272,8 @@ function render(children: React$Element<any>, options?: Options): Destination {
     null,
     options ? options.progressiveChunkSize : undefined,
     options ? options.onError : undefined,
-    options ? options.onCompleteAll : undefined,
-    options ? options.onCompleteShell : undefined,
+    options ? options.onAllReady : undefined,
+    options ? options.onShellReady : undefined,
   );
   ReactNoopServer.startWork(request);
   ReactNoopServer.startFlowing(request, destination);