-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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 TouchableOpacity componentDidUpdate causing an excessive number of pending callbacks #35387
Fix TouchableOpacity componentDidUpdate causing an excessive number of pending callbacks #35387
Conversation
Base commit: 4d7ddd4 |
Base commit: 4d7ddd4 |
PR build artifact for d3f397c is ready. |
@ryancat has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@gabrieldonadel when is this supposed to go live? It's still not in 0.71.2, I still have to use a patch for it... has it been missed maybe and never merged? |
@benomatis based on the changelog below this will most likely be released on 0.72 Line 206 in 728eef3
|
thank you @gabrieldonadel |
…f pending callbacks (#35387) Summary: The commit 3eddc9a included on v0.69 introduced a wrong `if` statement inside the `componentDidUpdate` function of the `TouchableOpacity` component. As this `if` statement always evaluates to `true` (`(true || false) !== undefined`) we end up making unnecessary calls to the `_opacityInactive` method every time the component props changes, e.g. every time a `<Text>` inside the TouchableOpacity changes we call this function over and over, and this has been causing some performance issues on big lists. This PR fixes this problem by adjusting the `componentDidUpdate` function to only call `_opacityInactive` when necessary. Closes #34442 Closes #32476 ## Changelog [General] [Fixed] - Fix TouchableOpacity componentDidUpdate causing an excessive number of pending callbacks Pull Request resolved: #35387 Test Plan: 1. Open the RNTester app and navigate to the `Touchable* and onPress` page 2. Test the `TouchableOpacity` component through the many sections Reviewed By: cipolleschi Differential Revision: D41397396 Pulled By: ryancat fbshipit-source-id: 24863b5cbbdd2f3dd1f654b43d7031560937b888
@benomatis : I just installed 0.71.4, and it seems to have the fix, FWIW. (Thanks, @gabrieldonadel, for fixing this. Spent a long time debugging my app until I saw this on the interwebs.) |
@fivecar Yes, I can confirm it's added to 0.71.4, thank you! |
…f pending callbacks (facebook#35387) Summary: The commit facebook@3eddc9a included on v0.69 introduced a wrong `if` statement inside the `componentDidUpdate` function of the `TouchableOpacity` component. As this `if` statement always evaluates to `true` (`(true || false) !== undefined`) we end up making unnecessary calls to the `_opacityInactive` method every time the component props changes, e.g. every time a `<Text>` inside the TouchableOpacity changes we call this function over and over, and this has been causing some performance issues on big lists. This PR fixes this problem by adjusting the `componentDidUpdate` function to only call `_opacityInactive` when necessary. Closes facebook#34442 Closes facebook#32476 ## Changelog [General] [Fixed] - Fix TouchableOpacity componentDidUpdate causing an excessive number of pending callbacks Pull Request resolved: facebook#35387 Test Plan: 1. Open the RNTester app and navigate to the `Touchable* and onPress` page 2. Test the `TouchableOpacity` component through the many sections Reviewed By: cipolleschi Differential Revision: D41397396 Pulled By: ryancat fbshipit-source-id: 24863b5cbbdd2f3dd1f654b43d7031560937b888
I am experiencing some issues opening a transparent modal with the keyboard opened. Also, I am listening to keyboard updates with 'will' and not 'did'. If I only listen 'didShow/didHide' events, the error disappear. Open keyboard -> Press TouchableOpacity button -> Open transparent modal -> Keyboard is implicitly dismissed (automatically) -> Close modal -> Keyboard is opened again automatically with buggy behaviour -> ERROR!
This is the error I am getting:
As a workaround, I am replacing the TouchableOpacity with the one provided by react native gesture handler. Any ideas? |
hey Victorio, have you found a solution? I'm running into this same exact error |
Temporary fix for me was to remove 'KeyboardAvoidingView' , as issue was only on iOS with this component |
Summary
The commit 3eddc9a included on v0.69 introduced a wrong
if
statement inside thecomponentDidUpdate
function of theTouchableOpacity
component. As thisif
statement always evaluates totrue
((true || false) !== undefined
) we end up making unnecessary calls to the_opacityInactive
method every time the component props changes, e.g. every time a<Text>
inside the TouchableOpacity changes we call this function over and over, and this has been causing some performance issues on big lists.This PR fixes this problem by adjusting the
componentDidUpdate
function to only call_opacityInactive
when necessary.Closes #34442
Closes #32476
Changelog
[General] [Fixed] - Fix TouchableOpacity componentDidUpdate causing an excessive number of pending callbacks
Test Plan
Touchable* and onPress
pageTouchableOpacity
component through the many sections