Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: restore execution context after RetryAfterError completed #21766

Merged
merged 6 commits into from
Jul 13, 2021
Merged

fix: restore execution context after RetryAfterError completed #21766

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1f3b991
test: Add failing test due to executionContext not being restored
eps1lon Jun 29, 2021
c591dc8
fix: restore execution context after RetryAfterError completed
eps1lon Jun 29, 2021
213babc
Poke codesandbox/ci
eps1lon Jun 30, 2021
dd36fa2
Merge branch 'main' into test/missing-act-after-error
eps1lon Jul 13, 2021
2c9bc48
Completely restore executionContext
eps1lon Jul 13, 2021
43699a6
expect a specific error
eps1lon Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,33 @@ function runActTests(label, render, unmount, rerender) {
]);
});

// @gate __DEV__
it('warns if a setState is called outside of act(...) after a component threw', () => {
let setValue = null;
function App({defaultValue}) {
if (defaultValue === undefined) {
throw new Error();
}
const [value, _setValue] = React.useState(defaultValue);
setValue = _setValue;
return value;
}

expect(() => {
act(() => {
render(<App defaultValue={undefined} />, container);
});
}).toThrow();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Add an expected message to toThrow(...) assertion

Copy link
Collaborator Author

@eps1lon eps1lon Jul 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done: 43699a6 (#21766)


act(() => {
rerender(<App defaultValue={0} />, container);
});

expect(() => setValue(1)).toErrorDev([
'An update to App inside a test was not wrapped in act(...).',
]);
});

describe('fake timers', () => {
beforeEach(() => {
jest.useFakeTimers();
Expand Down
4 changes: 4 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,8 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
lanes = errorRetryLanes;
exitStatus = renderRootSync(root, errorRetryLanes);
}

executionContext &= ~RetryAfterError;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the least intrusive change. I don't have all the context around what executionContext is used for. But I feel like it's missing a general "reset" step via executionContext = NoContext.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you do executionContext = prevExecutionContext instead? It should be impossible for the RetryAfterError bit to already be set, since we prevent nested renders, but I wouldn't want this pattern to spread elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done: 2c9bc48 (#21766)

}

if (exitStatus === RootFatalErrored) {
Expand Down Expand Up @@ -1006,6 +1008,8 @@ function performSyncWorkOnRoot(root) {
lanes = errorRetryLanes;
exitStatus = renderRootSync(root, lanes);
}

executionContext &= ~RetryAfterError;
}

if (exitStatus === RootFatalErrored) {
Expand Down
4 changes: 4 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,8 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
lanes = errorRetryLanes;
exitStatus = renderRootSync(root, errorRetryLanes);
}

executionContext &= ~RetryAfterError;
}

if (exitStatus === RootFatalErrored) {
Expand Down Expand Up @@ -1006,6 +1008,8 @@ function performSyncWorkOnRoot(root) {
lanes = errorRetryLanes;
exitStatus = renderRootSync(root, lanes);
}

executionContext &= ~RetryAfterError;
}

if (exitStatus === RootFatalErrored) {
Expand Down