Skip to content

[No QA] Refactor Search component to use useMemo for data calculation #51480

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.

Sign up for GitHub

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

Merged
luacmartins merged 1 commit into from
Oct 26, 2024
Merged

[No QA] Refactor Search component to use useMemo for data calculation #51480

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/components/Search/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {useIsFocused, useNavigation} from '@react-navigation/native';
import type {StackNavigationProp} from '@react-navigation/stack';
import React, {useCallback, useEffect, useRef, useState} from 'react';
import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {View} from 'react-native';
import type {NativeScrollEvent, NativeSyntheticEvent, StyleProp, ViewStyle} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
Expand Down Expand Up @@ -196,7 +196,13 @@ function Search({queryJSON, onSearchListScroll, contentContainerStyle}: SearchPr
const shouldShowLoadingMoreItems = !shouldShowLoadingState && searchResults?.search?.isLoading && searchResults?.search?.offset > 0;
const isSearchResultsEmpty = !searchResults?.data || SearchUIUtils.isSearchResultsEmpty(searchResults);
const prevIsSearchResultEmpty = usePrevious(isSearchResultsEmpty);
const data = searchResults === undefined ? [] : SearchUIUtils.getSections(type, status, searchResults.data, searchResults.search);
Copy link
Contributor Author

@blazejkustra blazejkustra Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: data was used as a dependency in a useEffect below, which caused the infinity loop (this component is not compiled because we ignore React rules 4 times, normally it would be taken care of)

Copy link
Contributor

@Kicu Kicu Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix.
Two things come to mind:

  • how come this did not surface previously?
  • I just remembered that there is a CONST.EMPTY_ARRAY for similar cases 😅 but useMemo feels better

Copy link
Contributor

@luacmartins luacmartins Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are great questions. I was asking myself the same thing since we didn't really change any behavior besides changing the react compiler.

Copy link
Contributor Author

@blazejkustra blazejkustra Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only explanation I came up with is that Search component was somehow compiled before updating the library 🤔

Copy link
Contributor

@Kicu Kicu Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @blazejkustra @luacmartins I finally found some time to investigate this and try to understand why did it break.
The theory that Błażej posted above is 100% correct.

I have checked out on a commit before the introduction of the updated (bumped?) react-compiler and run healthcheck then. The component Search/index.ts was previously compiled without any problems. And I think this compilation and memoization is what prevented the bug.
Then I checked out onto a commit that was before this fix but after updating react-compiler - and Search is no longer compiled. So this is the reason in the past there was no infinite effect but now there was.

Finally you can test yourself that Search is also not compiled on newest main. Here is the reason:

Failed to compile src/components/Search/index.tsx:182:78. Reason: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)


const data = useMemo(() => {
if (searchResults === undefined) {
return [];
}
return SearchUIUtils.getSections(type, status, searchResults.data, searchResults.search);
}, [searchResults, status, type]);

useEffect(() => {
/** We only want to display the skeleton for the status filters the first time we load them for a specific data type */
Expand Down