Skip to content

feat(react-dom): Add async act #37554

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

Merged
sheetalkamat merged 3 commits into from
Aug 19, 2019
Merged

feat(react-dom): Add async act #37554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
889c6f9
feat(react-dom): Add async act typings
eps1lon Aug 12, 2019
870a14c
Update version
eps1lon Aug 12, 2019
3b8bfaf
Normalize typing with react-test-renderer
eps1lon Aug 12, 2019
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
2 changes: 1 addition & 1 deletion types/react-dom/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Type definitions for React (react-dom) 16.8
// Type definitions for React (react-dom) 16.9
// Project: http://facebook.github.io/react/
// Definitions by: Asana <https://asana.com>
// AssureSign <http://www.assuresign.com>
Expand Down
5 changes: 2 additions & 3 deletions types/react-dom/react-dom-tests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,8 @@ describe('React dom test utils', () => {
it('accepts a sync callback that is void', () => {
ReactTestUtils.act(() => {});
});
it('rejects an async callback even if void', () => {
// $ExpectError
ReactTestUtils.act(async () => {});
it('accepts an async callback that is void', async () => {
await ReactTestUtils.act(async () => {});
});
it('rejects a callback that returns null', () => {
// $ExpectError
Expand Down
4 changes: 3 additions & 1 deletion types/react-dom/test-utils/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,10 @@ export function createRenderer(): ShallowRenderer;
* @see https://reactjs.org/blog/2019/02/06/react-v16.8.0.html#testing-hooks
*/
// the "void | undefined" is here to forbid any sneaky "Promise" returns.
// the actual return value is always a "DebugPromiseLike".
export function act(callback: () => void | undefined): DebugPromiseLike;
// the "void | undefined" is here to forbid any sneaky return values
// tslint:disable-next-line: void-return
export function act(callback: () => Promise<void | undefined>): Promise<undefined>;

@davidje13 davidje13 Aug 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this needs multiple signatures; DebugPromiseLike is promise-like, after all:

export function act(callback: () => Promise<void | undefined> | void | undefined): DebugPromiseLike;

should be fine. And I'm not sure if it even needs the whole void | undefined thing now that it doesn't need to catch promises in a special way. Maybe getting rid of the undefineds and just using voids is fine now? I don't really understand how that bit was working before. It could also be changed to simply () => any, but that doesn't get the intended semantics across so well.

@eps1lon eps1lon Aug 12, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

DebugPromiseLike is here to throw type errors because it will trigger react warnings if you await the sync act and more importantly can cause subtle scheduling bugs.

() => any is not valid either because react will warn if you return anything but a thenable from it.

@davidje13 davidje13 Aug 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, in that case I think the first signature should become a regular promise, because the recommendation now is to await even synchronous act (if you give it a synchronous function, it may still wait for asynchronous tasks). So maybe combine them with a Promise<void> return type?

@eps1lon eps1lon Aug 12, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

because the recommendation now is to await even synchronous act

Do you have a link for that? I remember a PR where the author for async act explicitly told me this can cause bugs and I'm fairly certain this still issues warnings from react.

@davidje13 davidje13 Aug 13, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, looks like I made it up, which is weird because I distinctly remember seeing that from a maintainer in the original tracker. Maybe it was an early thought which got edited out. Anyway doesn't seem to be the case, so I guess it does have to be 2 separate signatures.


// Intentionally doesn't extend PromiseLike<never>.
// Ideally this should be as hard to accidentally use as possible.
Expand Down
2 changes: 1 addition & 1 deletion types/react-dom/tslint.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@
"no-unnecessary-generics": false,
"no-unnecessary-type-assertion": false
}
}
}