Skip to content
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.

Sign up for GitHub

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

Turn off enableYieldingBeforePassive #31857

Merged
merged 4 commits into from
Dec 19, 2024
+44 −1
Merged

Turn off enableYieldingBeforePassive #31857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
32a2f86
Turn off `enableYieldingBeforePassive`
eps1lon Dec 19, 2024
a8efb5b
Add failing test
eps1lon Dec 19, 2024
9948ef5
Make dynamic
eps1lon Dec 19, 2024
841e658
Revert "Make dynamic"
eps1lon Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,4 +491,46 @@ describe('ReactSuspenseyCommitPhase', () => {
</>,
);
});

// FIXME: Should pass with `enableYieldingBeforePassive`
// @gate !enableYieldingBeforePassive
it('runs passive effects after suspended commit resolves', async () => {
function Effect() {
React.useEffect(() => {
Scheduler.log('flush effect');
});
return <Text text="render effect" />;
}

const root = ReactNoop.createRoot();

await act(() => {
root.render(
<Suspense fallback={<Text text="Loading..." />}>
<Effect />
<SuspenseyImage src="A" />
</Suspense>,
);
});

assertLog([
'render effect',
'Image requested [A]',
'Loading...',
'render effect',
]);
expect(root).toMatchRenderedOutput('Loading...');

await act(() => {
resolveSuspenseyThing('A');
});

assertLog(['flush effect']);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log is empty with enableYieldingBeforePassive turned on

expect(root).toMatchRenderedOutput(
<>
{'render effect'}
<suspensey-thing src="A" />
</>,
);
});
});
3 changes: 2 additions & 1 deletion packages/shared/ReactFeatureFlags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

actual.disableInputAttributeSyncing = __VARIANT__;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do that and tests fail though, just revert and I can look into enabling it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes please, but just doing that won't enable the variant tests, you can do that by adding it here:

Like this 9948ef5 (#31857)?

Copy link
Collaborator Author

@eps1lon eps1lon Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


export const enableLegacyCache = __EXPERIMENTAL__;

Expand Down
Loading