Flush act() only on exiting outermost call #15519
Conversation
Details of bundled changes.Comparing: c530639...8f88e96 react-noop-renderer
Generated by 🚫 dangerJS |
|
Will wait for #15591 to land before reviewing (or I can review now if you base this one on top of that one) |
| @@ -119,6 +118,13 @@ function act(callback: () => Thenable) { | |||
| called = true; | |||
| result.then( | |||
| () => { | |||
| if (actingUpdatesScopeDepth !== 1) { | |||
There was a problem hiding this comment.
Is there any scenario where actingUpdatesScopeDepth would decrement twice, and this would be less than 1? Maybe should change to actingUpdatesScopeDepth < 1 just in case.
There was a problem hiding this comment.
rather, actingUpdatesScopeDepth > 1
| @@ -285,14 +285,13 @@ describe('ReactTestUtils.act()', () => { | |||
| return ctr; | |||
| } | |||
| const el = document.createElement('div'); | |||
| act(() => { | |||
There was a problem hiding this comment.
Why did you move this to a separate act? I would expect moving the assertion outside act is sufficient.
There was a problem hiding this comment.
the effect wouldn't flush effects until the act exits, only after which the timeout in the component would start, failing the assertion. hence, 2 act()s.
There was a problem hiding this comment.
Why did you remove the await then?
| // a flash of "0" followed by "1" | ||
|
|
||
| // now you probably wouldn't write a test like this for your app | ||
| // you'd await act(async () => { ReactDOM.render(<App/> }) |
There was a problem hiding this comment.
So then why did you write it this way?
There was a problem hiding this comment.
like mentioned, just wanted to make sure the intermediate state was what I'd expected.
| expect(div.innerHTML).toBe('0'); | ||
| expect(ctr).toBe(1); | ||
|
|
||
| await act(async () => {}); // #just-react-things |
There was a problem hiding this comment.
Why did you remove the await from the earlier act?
There was a problem hiding this comment.
I wanted to specifically test the intermediate sync state. this test is named badly (and frankly on looking at it now, it doesn't seem super useful).
There was a problem hiding this comment.
It's not clear to me at all what behavior you're actually trying to test. More comments could help.
|
so many merge conflicts, so I redid it here #15682 |
Just starting this PR up to discuss the changes. The actual code changes aren't major. The big (breaking?) change is that nested acts won't work anymore.