Skip to content

Conversation

@ha3
Copy link

@ha3 ha3 commented Jul 15, 2023

This PR adds enabled prop to hooks, thus to allow controlling when the fetching will start. It is inspired by react query's same-named prop.

@ha3
Copy link
Author

ha3 commented Jul 24, 2023

Hi @marialungu, could you take a look at this, please? If you have any concerns, I can give you more context on why this is needed.

@raed667
Copy link
Collaborator

raed667 commented Aug 7, 2023

@ha3 We're looking into this feature!

Would you please provide more context on why this is needed? Can't the same logic be achieved in the application by respecting the rules of hooks and restructuring the code in a way to conditionally render the needed component where the hook is called?

@ha3 ha3 force-pushed the ha3/enabled-prop branch from 72b4ab7 to 2716648 Compare December 3, 2023 15:20
@ha3
Copy link
Author

ha3 commented Dec 3, 2023

@raed667 sorry for this very late response. I now rebased this branch with the next. I can give the below example to describe the necessity of this feature:

  1. Suppose you have a search screen, with trending items, recent search items, etc.
  2. You want to render these sections under a single component, like react native's SectionList which accepts a single data prop for all the sections
  3. Trending items are not always rendered, the decision is made according to a feature flag
const SearchScreen = () => {
  const recentSearches = useRecentSearches();
  const shouldRenderTrendingItems = useFeatureFlag('shouldRenderTrendingItems');
  const [trendingItems, setTrendingItems] = React.useState();
  
  const sectionData = React.useMemo(() => {
    const base = [];
    
    if (recentSearches) {
       base.push(recentSearches);
    }
    
    if (trendingItems) {
      base.push(trendingItems);
    }

    return base;
  }, [recentSearches, trendingItems])
  
  return (
    <>
      <SectionList
        data={sectionData}
      />
      {shouldRenderTrendingItems && <TrendingItems setTrendingItems={setTrendingItems} />
    </>
  )
};

/**
 * This component exists, just to get the trending items conditionally
 * without breaking the rules of hooks. With `enabled` prop, we can
 * move `useTrendingItems` back to the parent component and get rid of
 * the redundant sync logic.
*/
const TrendingItems = ({ setTrendingItems }) => {
  const trendingItems = useTrendingItems();

  React.useEffect(() => {
    setTrendingItems(trendingItems)
  }, [trendingItems])

  return null;
}

@ha3 ha3 force-pushed the ha3/enabled-prop branch from 2716648 to df22ca9 Compare April 7, 2024 11:10
@ha3
Copy link
Author

ha3 commented Apr 7, 2024

@raed667 @marialungu I synced the PR with the latest changes, could you please take a look at it? It is a non-breaking change that is not touching many places in the code and it would be difficult to achieve the functionality in the userland.

@raed667
Copy link
Collaborator

raed667 commented Apr 10, 2024

Hey @ha3 thanks for the update, and sorry for the late response. I raised this internally to our frontend libraries team and even though it makes a lot of sense, we agreed that the solution should be in the application code.

It is common in React code using Hooks, that one has to slightly change the structure of their code (usually just wrap some of it in another component) to respect the rules of Hooks.

This can be done as you have shown in your example by adding an extra "wrapper" component.

This can also be achieved by implementing your own custom hook, that wraps the Recommend API client @algolia/recommend like this:

import algoliarecommend from '@algolia/recommend';
const client = algoliarecommend('app_id', 'api_key');

const useCustomTrendingItems = ({ featureFlag }) => {
  const [results, setResults] = useState([]);

  useEffect(() => {
    if (featureFlag) {
      client
        .getTrendingItems([{ indexName: "index" }])
        .then((response) => {
          setResults(response.results || []);
        })
        .catch((error) => {
          setResults([]);
        });
    }
  }, [featureFlag]);

  if (featureFlag) {
    return { results };
  }
  return { results: [] };
};

On a more pragmatic level, as your PR looks good and fitting for your specific need, you can use your fork as I don't see this repository codebase diverging too much in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants