Warn when rendering tests in concurrent/sync mode without a mocked scheduler #16207
Conversation
Details of bundled changes.Comparing: e276a5e...e2858f6 react-art
react-dom
react-test-renderer
react-reconciler
react-native-renderer
react-noop-renderer
Generated by 🚫 dangerJS |
threepointone
force-pushed
the
warn-unmocked-scheduler-when-concurrent-batched
branch
from
bb8a6b4
to
78b5880
Compare
|
I don't think we're going to call it "batched mode" publicly. We use that term now to distinguish it from the legacy synchronous mode. But in the future, only "batched" and concurrent modes will exist. In that world, "batched" doesn't make sense because concurrent mode is also batched. So our current plan for the naming of the three modes is:
I sometimes refer to the two synchronous modes as "legacy sync" and "batched sync" if there's ambiguity. |
threepointone
force-pushed
the
warn-unmocked-scheduler-when-concurrent-batched
branch
from
78b5880
to
d5419a6
Compare
|
Noted, changed "Batched" for "Sync" in the messages. |
threepointone
changed the title
This PR adds a page on how to mock the scheduler, as a companion to facebook/react#16207. (Most people won't see this, since nobody's using concurrent/sync modes yet.)
threepointone
force-pushed
the
warn-unmocked-scheduler-when-concurrent-batched
branch
from
d5419a6
to
eb9908c
Compare
|
I moved the one in the act() definition into this new one, much prefer it this way. |
packages/react-dom/src/__tests__/ReactTestUtilsAct-test.internal.js
Outdated
Show resolved
Hide resolved
threepointone
force-pushed
the
warn-unmocked-scheduler-when-concurrent-batched
branch
3 times, most recently
from
048ac70
to
461c8a7
Compare
|
renamed some stuff for consistency |
threepointone
force-pushed
the
warn-unmocked-scheduler-when-concurrent-batched
branch
from
461c8a7
to
61e545f
Compare
| @@ -47,6 +48,9 @@ import { | |||
| scheduleSyncCallback, | |||
| } from './SchedulerWithReactIntegration'; | |||
|
|
|||
| // The scheduler is imported here *only* to detect whether it's been mocked | |||
| import * as Scheduler from 'scheduler'; | |||
There was a problem hiding this comment.
I could have also exported isSchedulerMocked from SchedulerWithReactIntegration.js if we want to prevent this import here.
threepointone
force-pushed
the
warn-unmocked-scheduler-when-concurrent-batched
branch
2 times, most recently
from
a33244e
to
9154e61
Compare
threepointone
force-pushed
the
warn-unmocked-scheduler-when-concurrent-batched
branch
from
9154e61
to
f2fbc75
Compare
|
ping! |
|
I find it weird that most developers will never interact with Scheduler, but we're asking them to mock it. If we provide them with a testing build of React DOM (and the other renderers), would that replace the need to mock the Scheduler package? I think it would in the normal case. The exception is if your framework interacts with Scheduler directly (instead of via built-in APIs like Btw, this doesn't affect 16.9, right? Since both |
|
So you’re right about your points in the short term. As you said, just the testing build will not be sufficient in the future, because a consumer may well use the scheduler directly via scheduler.next, etc. So in the future, we’ll end up asking them to mock the scheduler anyway. The reason I raised this PR, was to help early adopters with their tests. Assuming we won’t do another release for a few months, this could at least guide the shape of their tests and make it easier when we do make changes and demand a testing build etc. So let’s say, 6 months out, when we do ship, at least their tests wouldn’t break horribly. If we had testing builds right now, we could’ve suggested that, but this seemed like low effort-moderate win to be had now. Most, like 99%, of folks won’t see this warning, but the ones that do, tests will still work as expected for them. |
|
(This isn’t really blocking 16.9, happy to punt, but figured it would be a nice-to-have) |
|
btw, the linked doc (reactjs/react.dev#2171, preview at https://deploy-preview-2171--reactjs.netlify.com/docs/mock-scheduler.html) tries to make the role of the scheduler clearer |
threepointone
force-pushed
the
warn-unmocked-scheduler-when-concurrent-batched
branch
from
f2fbc75
to
591b50b
Compare
… scheduler Concurrent/Batched mode tests should always be run with a mocked scheduler (v17 or not). This PR adds a warning for the same. I'll put up a separate PR to the docs with a page detailing how to mock the scheduler.
threepointone
force-pushed
the
warn-unmocked-scheduler-when-concurrent-batched
branch
from
591b50b
to
e2858f6
Compare
|
pending item: land the doc PR, and set up the fb.me link. Will do. |
threepointone
deleted the
warn-unmocked-scheduler-when-concurrent-batched
branch
… scheduler (#16207) Concurrent/Batched mode tests should always be run with a mocked scheduler (v17 or not). This PR adds a warning for the same. I'll put up a separate PR to the docs with a page detailing how to mock the scheduler.
Concurrent/Sync mode tests should always be run with a mocked scheduler (v17 or not). This PR adds a warning for the same. I'll put up a separate PR to the docs with a page detailing how to mock the scheduler.