algolia / recommend

A UI library for Algolia Recommend, available for Vanilla JavaScript and React.
https://www.algolia.com/doc/ui-libraries/recommend/introduction/what-is-recommend/
MIT License
28 stars 11 forks source link

feat: add enabled prop #144

Open ha3 opened 1 year ago

ha3 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 12 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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.