act() - s / flushPassiveEffects / Scheduler.unstable_flushWithoutYielding
#15591
Conversation
a first crack at flushing the scheduler manually from inside act(). uses unstable_flushWithoutYielding(). The tests that changes, mostly replaced toFlushAndYield(...) with toHaveYielded(). For some tests that tested the state of the tree before flushing effects (but still after updates), I replaced act() with bacthedUpdates().
threepointone
changed the title
act() - s/flushPassiveEffects/Scheduler.unstable_flushWithoutYieldingact() - s / flushPassiveEffects / Scheduler.unstable_flushWithoutYielding
|
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: bb89b4e...763c03e react-dom
react-art
react-test-renderer
react-noop-renderer
scheduler
Generated by 🚫 dangerJS |
I agree the naming sucks. We'll rename them eventually; suggestions welcome. Need to throw a helpful error when Scheduler is not the mock build. |
of note, unstable_flushWithoutYielding now returns a boolean much like flushPassiveEffects
There was a problem hiding this comment.
Auto-flushing seems like an improvement!
I'm not sure how this would work with e.g. Jest environment where you have mock timers (which is important for certain types of things- like tests that verify Profiler timings). In those cases I think you would still end up with an awkward loop like this?
Do we have any plans for a way to hook into this to make sure mocked timers are run along with flushes?
|
@bvaughn my current thinking is to have a |
|
I guess I'll wait and see what you have in mind then. I assume For example, DevTools itself currently uses |
FWIW, I'm working on and off (mostly off...) on landing Lolex in Jest, which will add make sure Not that this affects you at all now, but possibly something to keep in mind. If you do want real timings while faking time you'd have to get a reference to the original |
|
I updated the PR to run the TestUtilsAct-test tests in both heritage/concurrent mode. I recommend turning off whitespace changes when reading. |
I also tried writing a test for it, but couldn't get the scheduler to unmock. included the failing test.
based on facebook#15591. of note, we don't modify our own tests to satisfy this warning, instead using a feature flag to disable the warning for some tests. this is because we're testing the actual sequencing of work in these tests, and don't want to flush everything with act().
|
opening this up for review, but still unsure how to get the scheduler to unmock. will keep poking at it. |
- pass the mock scheduler warning test, - rewrite some tests to use Scheduler.yieldValue - structure concurrent/legacy suites neatly
|
alright, I updated the PR
|
| @@ -347,14 +348,17 @@ describe('ReactHooks', () => { | |||
| }); | |||
| return <Child text={text} />; | |||
| } | |||
| let root; | |||
| act(() => { | |||
| root = ReactTestRenderer.create(null, {unstable_isConcurrent: true}); | |||
There was a problem hiding this comment.
Nit: If you move the ReactTestRenderer.create call outside act, does it warn? Maybe we can special case null? (I don't like how test renderer's create also updates at the same time, and I'm accustomed to passing null as a workaround.)
There was a problem hiding this comment.
It does not warn when you pass null, we only warn on update calls. It will warn if you try to render something with an effect ofc (once #15631 lands) To clarify, this is the behaviour you want, yeah? I'll move it out in this test.
|
alright, I'm going to land this. there are 2 major todos -
|
This simplifies your test helpers to loop until all timers are flushed (including the ones that get queued after updates), and works in concurrent mode. I also renamed actSuspense to actAsync to be clearer.
| "to the top of your tests, or in your framework's global config file -\n\n" + | ||
| 'As an example, for jest - \n' + | ||
| "jest.mock('scheduler', () => require.requireActual('scheduler/unstable_mock'));\n\n" + | ||
| 'For more info, visit https://fb.me/react-mock-scheduler', |
There was a problem hiding this comment.
FYI, this short URL was never created 😅 I get redirected to facebook.com
There was a problem hiding this comment.
Good catch, we should land reactjs/react.dev#2171 (preview: https://deploy-preview-2171--reactjs.netlify.com/docs/mock-scheduler.html)
…elding (#15591) * s/flushPassiveEffects/unstable_flushWithoutYielding a first crack at flushing the scheduler manually from inside act(). uses unstable_flushWithoutYielding(). The tests that changed, mostly replaced toFlushAndYield(...) with toHaveYielded(). For some tests that tested the state of the tree before flushing effects (but still after updates), I replaced act() with bacthedUpdates(). * ugh lint * pass build, flushPassiveEffects returns nothing now * pass test-fire * flush all work (not just effects), add a compatibility mode of note, unstable_flushWithoutYielding now returns a boolean much like flushPassiveEffects * umd build for scheduler/unstable_mock, pass the fixture with it * add a comment to Shcduler.umd.js for why we're exporting unstable_flushWithoutYielding * run testsutilsact tests in both sync/concurrent modes * augh lint * use a feature flag for the missing mock scheduler warning I also tried writing a test for it, but couldn't get the scheduler to unmock. included the failing test. * Update ReactTestUtilsAct-test.js - pass the mock scheduler warning test, - rewrite some tests to use Scheduler.yieldValue - structure concurrent/legacy suites neatly * pass failing tests in batchedmode-test * fix pretty/lint/import errors * pass test-build * nit: pull .create(null) out of the act() call
a first crack at flushing the scheduler manually from inside
act().unstable_flushWithoutYielding()directly instead offlushAll()toFlushAndYield(...)withtoHaveYielded().act()withbatchedUpdates().Open questions -
unstable_flushWithoutYieldingis an odd name, especially when it's followed bytoHaveYielded. Or am I doing something wrong here? Or should one of these named differently? edit: yup, the names are confusing. will change in a followup PRtodo