feat: remove redundant pressed state updates, if state is not used at… #44615
Conversation
facebook-github-bot
added
CLA Signed
| collapsable={false}> | ||
| {typeof children === 'function' ? children({pressed}) : children} | ||
| {isChildrenAFunction ? children({pressed}) : children} |
There was a problem hiding this comment.
This has a one drawback. Let's imagine this code:
<Pressable onPressIn={() => setPressed(true)} onPressOut={() => setPressed(false)}>{pressed ? renderTooltip : null}</Pressable>
It'll break after this change, however wrong it is.
This can be overcame by just not using function children and/or function styles conditionally, but that's what we know looking at the source code. I'd suggest JSDocing something like this -> Use function type [styles/children] to react to pressed state updates.
There was a problem hiding this comment.
One more example, more exotic but "less wrong", is when we switch Pressables on press in, but react reuses Pressable instance under the hood. This too, however, should be covered by a simple jsdoc reminding that function styles/children should only be used when reacting to pressed change.
There was a problem hiding this comment.
Adding this to changelog might also be a good idea.
There was a problem hiding this comment.
tbh I would love to have a simplest form of a Pressable exposed, without such features like styles/children as functions, so we could build on top of that "pure" Pressable such additions if needed. Yup, PR will change the behaviour slightly and I can ofc document it if the general idea of this PR is going to be accepted.
Maybe issue could be mitigated by storing press state in useRef but it's something I want to avoid.
There was a problem hiding this comment.
Definitely. I wonder if this utility is needed at all, it's really simple to implement for particular use case. Also +1 for the idea of building extra Pressable on top of this one. No idea how it could be called if stored in RN repo though.
| @@ -340,9 +345,9 @@ function Pressable(props: Props, forwardedRef): React.Node { | |||
| {...restPropsWithDefaults} | |||
| {...eventHandlers} | |||
| ref={mergedRef} | |||
| style={typeof style === 'function' ? style({pressed}) : style} | |||
There was a problem hiding this comment.
by making this suggested change: https://github.com/facebook/react-native/pull/44615/files#r1607362414 we could preserve previous version, that seems more readable to me. isStyleAFunction might be confusing for a moment, while typeof style === 'function' is instantly understandable.
There was a problem hiding this comment.
I've replied in #44615 (comment)
|
I love the idea of getting rid of extra overhead done by primitive components like Pressable. There's currently no way around this - whenever you use Pressable, you get that overhead. |
|
@fabriziocucci has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@fabriziocucci merged this pull request in cfa784c. |
|
This pull request was successfully merged by @Zahoq in cfa784c. When will my fix make it into a release? | How to file a pick request? |
…not functions (#44615) Summary: Goal of this PR is to optimise `Pressable` component, similarly to #724 . `Pressable` `style` and `children` properties can, but doesn't have to be functions. Usually we passing objects or arrays. `pressed` state is used only when `style` or `children` are `functions`, so let's update that state only in such case, otherwise let's skip state updates to improve the performance. That way we won't have to rerender the component when it is being pressed (assuming that `style` and `children` are not going to be functions) [GENERAL] [CHANGED] - Improve performance of `Pressable` component. Pull Request resolved: facebook/react-native#44615 Test Plan: Verify that `Pressable` updates its `pressed` state when `style` or `children` are functions. Reviewed By: javache Differential Revision: D57614309 Pulled By: fabriziocucci fbshipit-source-id: 473e0ab3c4bf7b3ef04ba19f76105ac65371a3fb
…not functions (facebook#44615) Summary: Goal of this PR is to optimise `Pressable` component, similarly to react-native-tvos/react-native-tvos#724 . `Pressable` `style` and `children` properties can, but doesn't have to be functions. Usually we passing objects or arrays. `pressed` state is used only when `style` or `children` are `functions`, so let's update that state only in such case, otherwise let's skip state updates to improve the performance. That way we won't have to rerender the component when it is being pressed (assuming that `style` and `children` are not going to be functions) ## Changelog: [GENERAL] [CHANGED] - Improve performance of `Pressable` component. Pull Request resolved: facebook#44615 Test Plan: Verify that `Pressable` updates its `pressed` state when `style` or `children` are functions. Reviewed By: javache Differential Revision: D57614309 Pulled By: fabriziocucci fbshipit-source-id: 473e0ab3c4bf7b3ef04ba19f76105ac65371a3fb
…not functions (#44615) Summary: Goal of this PR is to optimise `Pressable` component, similarly to #724 . `Pressable` `style` and `children` properties can, but doesn't have to be functions. Usually we passing objects or arrays. `pressed` state is used only when `style` or `children` are `functions`, so let's update that state only in such case, otherwise let's skip state updates to improve the performance. That way we won't have to rerender the component when it is being pressed (assuming that `style` and `children` are not going to be functions) [GENERAL] [CHANGED] - Improve performance of `Pressable` component. Pull Request resolved: facebook/react-native#44615 Test Plan: Verify that `Pressable` updates its `pressed` state when `style` or `children` are functions. Reviewed By: javache Differential Revision: D57614309 Pulled By: fabriziocucci fbshipit-source-id: 473e0ab3c4bf7b3ef04ba19f76105ac65371a3fb
Summary:
Goal of this PR is to optimise
Pressablecomponent, similarly to react-native-tvos/react-native-tvos#724 .Pressablestyleandchildrenproperties can, but doesn't have to be functions. Usually we passing objects or arrays.pressedstate is used only whenstyleorchildrenarefunctions, so let's update that state only in such case, otherwise let's skip state updates to improve the performance.That way we won't have to rerender the component when it is being pressed (assuming that
styleandchildrenare not going to be functions)Changelog:
[GENERAL] [CHANGED] - Improve performance of
Pressablecomponent.Test Plan:
Verify that
Pressableupdates itspressedstate whenstyleorchildrenare functions or objects/arrays.