diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js index af5f3d9b3565a..3df7eb79de6cc 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js @@ -17,7 +17,7 @@ let React; let ReactDOMFizzServer; let Suspense; -describe('ReactDOMFizzServer', () => { +describe('ReactDOMFizzServerBrowser', () => { beforeEach(() => { jest.resetModules(); React = require('react'); @@ -209,6 +209,113 @@ describe('ReactDOMFizzServer', () => { ]); }); + it('should reject if aborting before the shell is complete', async () => { + const errors = []; + const controller = new AbortController(); + const promise = ReactDOMFizzServer.renderToReadableStream( +
+ +
, + { + signal: controller.signal, + onError(x) { + errors.push(x.message); + }, + }, + ); + + await jest.runAllTimers(); + + const theReason = new Error('aborted for reasons'); + // @TODO this is a hack to work around lack of support for abortSignal.reason in node + // The abort call itself should set this property but since we are testing in node we + // set it here manually + controller.signal.reason = theReason; + controller.abort(theReason); + + let caughtError = null; + try { + await promise; + } catch (error) { + caughtError = error; + } + expect(caughtError).toBe(theReason); + expect(errors).toEqual(['aborted for reasons']); + }); + + it('should be able to abort before something suspends', async () => { + const errors = []; + const controller = new AbortController(); + function App() { + controller.abort(); + return ( + Loading}> + + + ); + } + const streamPromise = ReactDOMFizzServer.renderToReadableStream( +
+ +
, + { + signal: controller.signal, + onError(x) { + errors.push(x.message); + }, + }, + ); + + let caughtError = null; + try { + await streamPromise; + } catch (error) { + caughtError = error; + } + expect(caughtError.message).toBe( + 'The render was aborted by the server without a reason.', + ); + expect(errors).toEqual([ + 'The render was aborted by the server without a reason.', + ]); + }); + + it('should reject if passing an already aborted signal', async () => { + const errors = []; + const controller = new AbortController(); + const theReason = new Error('aborted for reasons'); + // @TODO this is a hack to work around lack of support for abortSignal.reason in node + // The abort call itself should set this property but since we are testing in node we + // set it here manually + controller.signal.reason = theReason; + controller.abort(theReason); + + const promise = ReactDOMFizzServer.renderToReadableStream( +
+ Loading
}> + + + , + { + signal: controller.signal, + onError(x) { + errors.push(x.message); + }, + }, + ); + + // Technically we could still continue rendering the shell but currently the + // semantics mean that we also abort any pending CPU work. + let caughtError = null; + try { + await promise; + } catch (error) { + caughtError = error; + } + expect(caughtError).toBe(theReason); + expect(errors).toEqual(['aborted for reasons']); + }); + it('should not continue rendering after the reader cancels', async () => { let hasLoaded = false; let resolve; @@ -226,7 +333,7 @@ describe('ReactDOMFizzServer', () => { const stream = await ReactDOMFizzServer.renderToReadableStream(
Loading
}> - /> + , { @@ -296,7 +403,7 @@ describe('ReactDOMFizzServer', () => { expect(result).toMatchInlineSnapshot(`"
${str2049}
"`); }); - it('Supports custom abort reasons with a string', async () => { + it('supports custom abort reasons with a string', async () => { const promise = new Promise(r => {}); function Wait() { throw promise; @@ -337,7 +444,7 @@ describe('ReactDOMFizzServer', () => { expect(errors).toEqual(['foobar', 'foobar']); }); - it('Supports custom abort reasons with an Error', async () => { + it('supports custom abort reasons with an Error', async () => { const promise = new Promise(r => {}); function Wait() { throw promise; diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js index f5f85c953148a..f3a6aa50c3fdf 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js @@ -15,7 +15,7 @@ let React; let ReactDOMFizzServer; let Suspense; -describe('ReactDOMFizzServer', () => { +describe('ReactDOMFizzServerNode', () => { beforeEach(() => { jest.resetModules(); React = require('react'); @@ -166,7 +166,6 @@ describe('ReactDOMFizzServer', () => {
, - { onError(x) { reportedErrors.push(x); @@ -232,7 +231,6 @@ describe('ReactDOMFizzServer', () => { , - { onError(x) { reportedErrors.push(x); @@ -288,7 +286,6 @@ describe('ReactDOMFizzServer', () => { , - { onError(x) { errors.push(x.message); @@ -315,6 +312,49 @@ describe('ReactDOMFizzServer', () => { expect(isCompleteCalls).toBe(1); }); + it('should fail the shell if you abort before work has begun', async () => { + let isCompleteCalls = 0; + const errors = []; + const shellErrors = []; + const {writable, output, completed} = getTestWritable(); + const {pipe, abort} = ReactDOMFizzServer.renderToPipeableStream( +
+ Loading
}> + + + , + { + onError(x) { + errors.push(x.message); + }, + onShellError(x) { + shellErrors.push(x.message); + }, + onAllReady() { + isCompleteCalls++; + }, + }, + ); + pipe(writable); + + // Currently we delay work so if we abort, we abort the remaining CPU + // work as well. + + // Abort before running the timers that perform the work + const theReason = new Error('uh oh'); + abort(theReason); + + jest.runAllTimers(); + + await completed; + + expect(errors).toEqual(['uh oh']); + expect(shellErrors).toEqual(['uh oh']); + expect(output.error).toBe(theReason); + expect(output.result).toBe(''); + expect(isCompleteCalls).toBe(0); + }); + it('should be able to complete by abort when the fallback is also suspended', async () => { let isCompleteCalls = 0; const errors = []; @@ -327,7 +367,6 @@ describe('ReactDOMFizzServer', () => { , - { onError(x) { errors.push(x.message); diff --git a/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js b/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js index fc35fdb286798..acfe13e4ebbcc 100644 --- a/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js +++ b/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js @@ -96,11 +96,15 @@ function renderToReadableStream( ); if (options && options.signal) { const signal = options.signal; - const listener = () => { + if (signal.aborted) { abort(request, (signal: any).reason); - signal.removeEventListener('abort', listener); - }; - signal.addEventListener('abort', listener); + } else { + const listener = () => { + abort(request, (signal: any).reason); + signal.removeEventListener('abort', listener); + }; + signal.addEventListener('abort', listener); + } } startWork(request); }); diff --git a/packages/react-dom/src/server/ReactDOMLegacyServerImpl.js b/packages/react-dom/src/server/ReactDOMLegacyServerImpl.js index 27e41b42e43ae..504201f3c24ff 100644 --- a/packages/react-dom/src/server/ReactDOMLegacyServerImpl.js +++ b/packages/react-dom/src/server/ReactDOMLegacyServerImpl.js @@ -76,7 +76,7 @@ function renderToStringImpl( // That way we write only client-rendered boundaries from the start. abort(request, abortReason); startFlowing(request, destination); - if (didFatal) { + if (didFatal && fatalError !== abortReason) { throw fatalError; } diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 9e6551d938c0f..a8a3fe6932072 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -1530,7 +1530,7 @@ function abortTaskSoft(task: Task): void { finishedTask(request, boundary, segment); } -function abortTask(task: Task, request: Request, reason: mixed): void { +function abortTask(task: Task, request: Request, error: mixed): void { // This aborts the task and aborts the parent that it blocks, putting it into // client rendered mode. const boundary = task.blockedBoundary; @@ -1541,35 +1541,30 @@ function abortTask(task: Task, request: Request, reason: mixed): void { request.allPendingTasks--; // We didn't complete the root so we have nothing to show. We can close // the request; - if (request.status !== CLOSED) { - request.status = CLOSED; - if (request.destination !== null) { - close(request.destination); - } + if (request.status !== CLOSING && request.status !== CLOSED) { + logRecoverableError(request, error); + fatalError(request, error); } } else { boundary.pendingTasks--; if (!boundary.forceClientRender) { boundary.forceClientRender = true; - let error = - reason === undefined - ? new Error('The render was aborted by the server without a reason.') - : reason; boundary.errorDigest = request.onError(error); if (__DEV__) { const errorPrefix = 'The server did not finish this Suspense boundary: '; + let errorMessage; if (error && typeof error.message === 'string') { - error = errorPrefix + error.message; + errorMessage = errorPrefix + error.message; } else { // eslint-disable-next-line react-internal/safe-string-coercion - error = errorPrefix + String(error); + errorMessage = errorPrefix + String(error); } const previousTaskInDev = currentTaskInDEV; currentTaskInDEV = task; try { - captureBoundaryErrorDetailsDev(boundary, error); + captureBoundaryErrorDetailsDev(boundary, errorMessage); } finally { currentTaskInDEV = previousTaskInDev; } @@ -1582,7 +1577,7 @@ function abortTask(task: Task, request: Request, reason: mixed): void { // If this boundary was still pending then we haven't already cancelled its fallbacks. // We'll need to abort the fallbacks, which will also error that parent boundary. boundary.fallbackAbortableTasks.forEach(fallbackTask => - abortTask(fallbackTask, request, reason), + abortTask(fallbackTask, request, error), ); boundary.fallbackAbortableTasks.clear(); @@ -2178,8 +2173,14 @@ export function startFlowing(request: Request, destination: Destination): void { export function abort(request: Request, reason: mixed): void { try { const abortableTasks = request.abortableTasks; - abortableTasks.forEach(task => abortTask(task, request, reason)); - abortableTasks.clear(); + if (abortableTasks.size > 0) { + const error = + reason === undefined + ? new Error('The render was aborted by the server without a reason.') + : reason; + abortableTasks.forEach(task => abortTask(task, request, error)); + abortableTasks.clear(); + } if (request.destination !== null) { flushCompletedQueues(request, request.destination); }