Turn off enableYieldingBeforePassive
#31857
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
eps1lon
force-pushed
the
disable-yielding-before-passive
branch
from
209b425 to
32a2f86
Compare
| resolveSuspenseyThing('A'); | ||
| }); | ||
|
|
||
| assertLog(['flush effect']); |
There was a problem hiding this comment.
log is empty with enableYieldingBeforePassive turned on
| @@ -78,7 +78,8 @@ export const enableLegacyFBSupport = false; | |||
| // ----------------------------------------------------------------------------- | |||
|
|
|||
| // Yield to the browser event loop and not just the scheduler event loop before passive effects. | |||
| export const enableYieldingBeforePassive = __EXPERIMENTAL__; | |||
| // Fix gated tests that fail with this flag enabled before turning it back on. | |||
| export const enableYieldingBeforePassive = false; | |||
There was a problem hiding this comment.
Should I add this as a variant to ReactFeatureFlags.www-dynamic.js in the meantime even though there is no GK associated with it yet?
There was a problem hiding this comment.
yes please, but just doing that won't enable the variant tests, you can do that by adding it here:
react/scripts/jest/setupTests.www.js
Line 13 in 1e9ef39
There was a problem hiding this comment.
If you do that and tests fail though, just revert and I can look into enabling it
There was a problem hiding this comment.
Just a heads up, the RN sync is blocked due to failing tests (likely the same bug from your unit test since its asserting a function from an effect was called after resolving some Relay data) since this change leaked into the Scheduler-dev build there.
I just merged #31859 to unblock the sync. But it may make sense to just revert this flag to false until we have a fix.
There was a problem hiding this comment.
likely the same bug from your unit test since its asserting a function from an effect was called after resolving some Relay data
That might actually be an expected one where they previously relied on passive effects being flushed synchronously. Do they wrap the function that resolves the promise in act()?
There was a problem hiding this comment.
yes please, but just doing that won't enable the variant tests, you can do that by adding it here:
Like this 9948ef5 (#31857)?
There was a problem hiding this comment.
Seems like we didn't gate a bunch of tests on this: https://github.com/facebook/react/actions/runs/12418967266/job/34673303185?pr=31857. Reverting the variant change for now to unblock the sync.
This reverts commit 9948ef5. A bunch of tests weren't gated on this behavior.
The flag causes passive effects to not fire if they were part of a suspensey commit. Potential forward-fix in #31858
test plan