chore(performance): optimize style props #1183
Conversation
hgray-instawork
force-pushed
the
hardin/render-style-hooks
branch
from
fb45336 to
2611fd9
Compare
flochtililoch
left a comment
flochtililoch
left a comment
There was a problem hiding this comment.
I'm not sure if you did a pass already to address what we discussed yesterday, but re-adding my feedback here (with some additional feedback):
I think we should remove the memoization for:
- simple attribute getters or objects accessor
- any low usage components (i.e. all form elements, webview)
Right now, memoization for these adds a lot of the verbosity in code, and I doubt they provide a real gain. I think memoization makes sense in some places, for heavy usage components like text, or view, and when there's computation involved (for loop, object creation etc.).
Going one step further in this migration, could we:
- define a return type for the new hooks, and remove use of
anywhen these are used - convert
createTestPropstouseTestProps, have it do internal memoization on element - remove the code within legacy
create*functions and have these delegate to their equivalent hook function (granted we would also need to silence the ESLint warning about not being in a React component)
And as future tasks (low priority but we should track):
- I see HvTextField disables rules of hooks linting rule, or does unnecessary re-creation of objects (
elementPropsis spread intop, which is spread again when applied to the component instance) - I noted many components (
hv-switch,hv-select-single) were reading attributes directly fromelement, but we're also able to get these attributes when callinguseProps, so we should probably refactor to simplify and remove duplication.
| options: HvComponentOptions, | ||
| ) => { | ||
| const { attributes } = element; | ||
| const { focused, pressed, pressedSelected, selected, styleAttr } = options; |
There was a problem hiding this comment.
minor: it seems there's no need to de-structure this object and recreate it on the next line
| const component = useMemo(() => React.createElement(Image, componentProps), [ | ||
| componentProps, | ||
| ]); |
There was a problem hiding this comment.
component is changing from the return value of React.createElement (previous implementation) to a functional component that returns the returned value of React.createElement (new implementation). I assume this is unintentional, as this changes the produced tree, and I wonder if we need this.
| * Component used to render the Cancel/Done buttons in the picker modal. | ||
| */ | ||
| export default (props: Props) => { | ||
| // eslint-disable-next-line react/destructuring-assignment |
There was a problem hiding this comment.
minor/may be addressed in a later PR: I think we should disable that rule in the eslint config since it's so common to do prop destructuring in FC
| const color = props['activity-indicator-color'] || '#8d9494'; | ||
| const loadBehavior = props['show-loading-indicator']; | ||
| let injectedJavaScript = props['injected-java-script']; | ||
| const webViewProps = useMemo(() => { |
There was a problem hiding this comment.
I feel this memoization isn't very useful: it does not wrap any complex computation, and the entirety of what's generated here comes from componentProps which is already heavily memoized within useProps.
| if (props.element.getAttribute('hide') === 'true') { | ||
| // eslint-disable-next-line react/destructuring-assignment | ||
| const { element, onUpdate, options, stylesheets } = props; | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks |
There was a problem hiding this comment.
which warning are we disabling here?
| styleAttr: 'field-text-style', | ||
| }); | ||
|
|
||
| const { testID, accessibilityLabel } = useMemo( |
There was a problem hiding this comment.
per my comment re createTestProps → useTestProps, we shouldn't need the memoization here once applied
| [element], | ||
| ); | ||
|
|
||
| const placeholderTextColor: DOMString | null | undefined = useMemo( |
There was a problem hiding this comment.
this feels overkill too
| const needsEmptyOption = useMemo( | ||
| () => pickerItems.length > 0 && pickerItems[0].getAttribute('value') !== '', | ||
| [pickerItems], | ||
| ); | ||
|
|
||
| const placeholder = useMemo( | ||
| () => element.getAttribute('placeholder') || undefined, | ||
| [element], | ||
| ); |
| // Gets all of the <picker-item> elements. All picker item elements | ||
| // with a value and label are turned into options for the picker. | ||
| const children = useMemo(() => { | ||
| const items = pickerItems.reduce<Array<React.ReactNode>>((acc, item) => { |
There was a problem hiding this comment.
did we need to change the logic from a map to an array reduce?
| this.props.element.getAttribute('preformatted') === 'true', | ||
| }, | ||
| // TODO: Replace with <HvChildren> | ||
| const component = useMemo( |
There was a problem hiding this comment.
this memo will be re-created anytime the props changes - wouldn't we be better off memoizing the entire component?
Optimizing style and props creation by replacing
createPropsandcreateStylePropwith hooks. After converting several class functions into functional components, the hooks were added. Additional optimizations include:useMemoanduseCallback. Grouped options and values together where possible to reduce overhead.See commits for full breakdown.
Asana