-
Notifications
You must be signed in to change notification settings - Fork 50.4k
[flags] add enableEffectEventMutationPhase #35548
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Comparing: be3fb29...5804ed3 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
| ...(gate('enableViewTransition') && | ||
| !gate('enableEffectEventMutationPhase') | ||
| ? ['Child Layout Create A 3 2 B 1 1 1'] | ||
| : ['Child Layout Create A 3 2 B 3 2 2']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebmarkbage can you review these tests and let me know if you think the first or second logs are right? Inside enableViewTransition, when a tree goes from hidden to visible we were not updating the useEffectEvent callback ref.
| }); | ||
|
|
||
| // @gate enableActivity | ||
| it('effect events are fresh in deeply nested hidden Activities', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebmarkbage this is a simplified test.
Small optimization for useEffectEvent. Not sure we even need a flag for it, but it will be a nice killswitch.
As an added benefit, it fixes a bug when
enableViewTransitionis on, where we were not updating the useEffectEvent callback when a tree went from hidden to visible.