-
Notifications
You must be signed in to change notification settings - Fork 24.2k
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
Fix Switch causing RetryableMountingLayerException #32602
Conversation
Summary: Changelog: Change useLayoutEffect to useEffect to fix facebook#32594 - stuck operation in mViewCommandOperations list in Android Release
Base commit: 05b4570 |
Base commit: 05b4570 |
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.
In the issue, you mention that we attempt to setNativeValue
on initial render but wouldn't nativeSwitchRef.current
be null in this case?
I believe that's why we have the check there to avoid the case of the native view not being rendered yet. If this isn't the appropriate check, then maybe we need to update it.
For more context, this was part of a Switch refactor and this logic used to live in a
useLayoutEffect as this behavior we're trying to do is keep the JS and native value in sync but maybe we need to also check if nativeValue is set.
Although, forgive me if I'm mis-remembering, shouldn't the native ref only be set when the native view has been rendered? |
@jonathanmos ping? |
@lunaleaps I'd like to revive this pr and hopefully we can bring it to a conclusion. You suggested in your comments adding a check for whether native.value was set. I've tried adding such a check to shouldUpdateNativeSwitch and it does solve the issue, but I've hesitated to push it as a fix as i noticed that after this change shouldUpdateNativeSwitch seems to always return false (value is always equal to native.value) and so setNativeValue is not being called. I think I'm not understanding completely in what situation these values are both set but different. Do you know what steps I can use to simulate such a situation in a test to ensure that everything works as expected? |
Regarding the nativeref being set only on render, I'm unsure exactly what is going on here. The nativeref is undoubtedly set when the call to setnativevalue is made, but nevertheless causes a retryableMountingException on the initial render |
Hi @lunaleaps 👋🏼 |
Hmm for coming up with a case where native might be out of sync with JS, I think is an edge case. I could see something like dynamically toggling Are you seeing other issues when adding a null check for |
@lunaleaps In the tests that I've run, out of the variables in I've tried rapid toggling of disabled state for the switch, as well as sending many js operations, but was unable to produce a situation where sync was lost. |
045de89
to
71a588b
Compare
@lunaleaps has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@jonathanmos Can we update the changelog summary of this change as well? Thanks! |
@lunaleaps I've updated the changelog for the issue |
@lunaleaps has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Oh actually thinking about this again, I think the case to test this is if we hardcode the JS value to be some constant value -- then we want to ignore any attempts of native to toggle the switch. Let me try this case out |
This pull request was successfully merged by @jonathanmos in 8d50bf1. When will my fix make it into a release? | Upcoming Releases |
Summary: Added a null check to native.value in Switch to fix regression from RN 66 -> stuck operation in mViewCommandOperations list in Android Release on initial layout of a screen with Switch component. If approved, please incorporate this fix into an RN 66 release. ## Changelog [Android][Fixed] - Added a null check to native.value in Switch to fix #32594 Pull Request resolved: #32602 Test Plan: To reproduce, put a log in UIViewOperationQueue in dispatchViewUpdates you can see that the RetryableMountingException is no longer thrown for a screen with the Switch component on Android Release. As a result, mViewCommandOperations no longer has a stuck operation on initial layout. Reviewed By: charlesbdudley Differential Revision: D34397788 Pulled By: lunaleaps fbshipit-source-id: 1cee3516fb987942dfa50ad1f2d11c965a947f05
Summary: Added a null check to native.value in Switch to fix regression from RN 66 -> stuck operation in mViewCommandOperations list in Android Release on initial layout of a screen with Switch component. If approved, please incorporate this fix into an RN 66 release. ## Changelog [Android][Fixed] - Added a null check to native.value in Switch to fix #32594 Pull Request resolved: #32602 Test Plan: To reproduce, put a log in UIViewOperationQueue in dispatchViewUpdates you can see that the RetryableMountingException is no longer thrown for a screen with the Switch component on Android Release. As a result, mViewCommandOperations no longer has a stuck operation on initial layout. Reviewed By: charlesbdudley Differential Revision: D34397788 Pulled By: lunaleaps fbshipit-source-id: 1cee3516fb987942dfa50ad1f2d11c965a947f05
Summary: Added a null check to native.value in Switch to fix regression from RN 66 -> stuck operation in mViewCommandOperations list in Android Release on initial layout of a screen with Switch component. If approved, please incorporate this fix into an RN 66 release. ## Changelog [Android][Fixed] - Added a null check to native.value in Switch to fix facebook#32594 Pull Request resolved: facebook#32602 Test Plan: To reproduce, put a log in UIViewOperationQueue in dispatchViewUpdates you can see that the RetryableMountingException is no longer thrown for a screen with the Switch component on Android Release. As a result, mViewCommandOperations no longer has a stuck operation on initial layout. Reviewed By: charlesbdudley Differential Revision: D34397788 Pulled By: lunaleaps fbshipit-source-id: 1cee3516fb987942dfa50ad1f2d11c965a947f05
Summary
Added a null check to native.value in Switch to fix regression from RN 66 -> stuck operation in mViewCommandOperations list in Android Release on initial layout of a screen with Switch component. If approved, please incorporate this fix into an RN 66 release.
Changelog
[Android][Fixed] - Added a null check to native.value in Switch to fix #32594
Test Plan
To reproduce, put a log in UIViewOperationQueue in dispatchViewUpdates you can see that the RetryableMountingException is no longer thrown for a screen with the Switch component on Android Release. As a result, mViewCommandOperations no longer has a stuck operation on initial layout.