-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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.
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
await act(async () => ...) #14853
await act(async () => ...) #14853
Conversation
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: 9307932...e49e043 react-dom
react-art
react-native-renderer
react-test-renderer
react-noop-renderer
react-reconciler
Generated by 🚫 dangerJS |
act = ReactTestUtils.act; | ||
}); | ||
|
||
describe('sync', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are the ones moved in from TestUtils-test
); | ||
}); | ||
}); | ||
describe('async', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are newly added
}); | ||
|
||
await sleep(1000); | ||
expect(console.error).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but nuh uh, we warn anyway (todo - match on actual warning message)
Assuming there’s no big problem with the approach, I’m going to close this pr later today and continue working on it in my branch. Please feel free to leave comments! |
(nevermind, I'll leave it open) |
@@ -517,173 +515,4 @@ describe('ReactTestUtils', () => { | |||
ReactTestUtils.renderIntoDocument(<Component />); | |||
expect(mockArgs.length).toEqual(0); | |||
}); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved these tests into a separate file for .act tests
spoke with dan, moving the |
The failing test is odd, investigating. |
it's an odd one, because not only does sync act not flush effects correctly, but the async one does (wut). verified it's fine with the dom version.
* Initialized wasm submodule * Add npm scripts and webpack config to compile wasm and include to assets * Update express to fix mime type issue with wasm * Update react to fix async act (see facebook/react#14853) * Changed error message component props for less tight coupling
TODO -
(repasting from the group)
I hacked together an asynchronous version of
act(...)
, and it's kinda nice.You've seen the synchronous version -
This still works, and gives the same warnings. But if you pass an
async
function -Neat! I set it up so if you don't
await
the result fromact
, a warning gets triggered (withsetImmediate
) to do so. That makes it a bit harder to have rogue asyncact()
calls in the ether.You can nest
act()
calls -I implemented a cheap form of unrolling safety, so if a previous
act()
gets closed before any subsequentact()
calls, a warning gets triggered. This should prevent most interleaving attempts, and maintain a tree-like shape ofact()
blocks.pros -
cons -
act()
call... which might be fine?)