Skip to content
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.

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

feat: add enabled prop #144

Open
wants to merge 1 commit into
base: next
Choose a base branch
from
Open

feat: add enabled prop #144

wants to merge 1 commit into from

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
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
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.

None yet

2 participants