Skip to content

Commit

Permalink
Fix state leaking when a function component throws on server render (#…
Browse files Browse the repository at this point in the history
…19212)

* add unit test asserting internal hooks state is reset

* Reset internal hooks state before rendering

* reset hooks state on error

* Use expect...toThrow instead of try/catch in test

* reset dev-only hooks state inside resetHooksState

* reset currentlyRenderingComponent to null
  • Loading branch information
pmaccart committed Jul 8, 2020
1 parent 6fd4321 commit b85b476
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 13 deletions.
Expand Up @@ -867,6 +867,44 @@ describe('ReactDOMServerHooks', () => {
});
});

it('renders successfully after a component using hooks throws an error', () => {
function ThrowingComponent() {
const [value, dispatch] = useReducer((state, action) => {
return state + 1;
}, 0);

// throw an error if the count gets too high during the re-render phase
if (value >= 3) {
throw new Error('Error from ThrowingComponent');
} else {
// dispatch to trigger a re-render of the component
dispatch();
}

return <div>{value}</div>;
}

function NonThrowingComponent() {
const [count] = useState(0);
return <div>{count}</div>;
}

// First, render a component that will throw an error during a re-render triggered
// by a dispatch call.
expect(() => ReactDOMServer.renderToString(<ThrowingComponent />)).toThrow(
'Error from ThrowingComponent',
);

// Next, assert that we can render a function component using hooks immediately
// after an error occurred, which indictates the internal hooks state has been
// reset.
const container = document.createElement('div');
container.innerHTML = ReactDOMServer.renderToString(
<NonThrowingComponent />,
);
expect(container.children[0].textContent).toEqual('0');
});

if (__EXPERIMENTAL__) {
describe('useOpaqueIdentifier', () => {
it('generates unique ids for server string render', async () => {
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Expand Up @@ -60,6 +60,7 @@ import escapeTextForBrowser from './escapeTextForBrowser';
import {
prepareToUseHooks,
finishHooks,
resetHooksState,
Dispatcher,
currentPartialRenderer,
setCurrentPartialRenderer,
Expand Down Expand Up @@ -955,6 +956,7 @@ class ReactDOMServerRenderer {
} finally {
ReactCurrentDispatcher.current = prevDispatcher;
setCurrentPartialRenderer(prevPartialRenderer);
resetHooksState();
}
}

Expand Down
24 changes: 11 additions & 13 deletions packages/react-dom/src/server/ReactPartialRendererHooks.js
Expand Up @@ -202,24 +202,22 @@ export function finishHooks(

children = Component(props, refOrContext);
}
resetHooksState();
return children;
}

// Reset the internal hooks state if an error occurs while rendering a component
export function resetHooksState(): void {
if (__DEV__) {
isInHookUserCodeInDev = false;
}

currentlyRenderingComponent = null;
didScheduleRenderPhaseUpdate = false;
firstWorkInProgressHook = null;
numberOfReRenders = 0;
renderPhaseUpdates = null;
workInProgressHook = null;
if (__DEV__) {
isInHookUserCodeInDev = false;
}

// These were reset above
// currentlyRenderingComponent = null;
// didScheduleRenderPhaseUpdate = false;
// firstWorkInProgressHook = null;
// numberOfReRenders = 0;
// renderPhaseUpdates = null;
// workInProgressHook = null;

return children;
}

function readContext<T>(
Expand Down

0 comments on commit b85b476

Please sign in to comment.