hyva-themes / magento2-react-checkout

Highly Customizable Checkout for Magento 2, Built with React.
BSD 3-Clause "New" or "Revised" License
180 stars 54 forks source link

Weird async method in useEffect (obsolete IIFE?) #275

Closed DanielRuf closed 2 years ago

DanielRuf commented 2 years ago

https://github.com/hyva-themes/magento2-react-checkout/blob/main/src/reactapp/src/components/CheckoutForm/CheckoutForm.jsx#L48-L50

https://github.com/hyva-themes/magento2-react-checkout/blob/main/src/reactapp/src/components/CheckoutForm/CheckoutForm.jsx#L33-L34

It should be probably look like this:

  useEffect(async () => {
    try {
      setPageLoader(true);
      const data = await aggregatedQueryRequest(appDispatch);
      await storeAggregatedCartStates(data);
      await storeAggregatedAppStates(data);
      setInitialData(data);
      setPageLoader(false);
    } catch (error) {
      setPageLoader(false);
    }
  }, [
    appDispatch,
    // setPageLoader, // can be removed since it is not a dependency that changes its value?
    // storeAggregatedAppStates, // can be removed since it is not a dependency that changes its value?
    // storeAggregatedCartStates, // can be removed since it is not a dependency that changes its value?
  ]);
rajeev-k-tomy commented 2 years ago

This is actually a fantastic question. You are right that we don't actually needed those dependencies there including appDispatch. Because appDispatch is the reference for the dispatch method of the reducer used in AppProvider.

But, I included them in the dependencies because the CheckoutForm component does not know they are actually reducer dispatch actions and they will be referred only once. Those variables are coming from the custom hooks in this example. So it is necessary we included them in the dependency array.

Why? because there is a rule react-hooks/exhaustive-deps which cries if you didn't include all of the dependencies in the array. So as a standard, it is always better we stick with that rule.

But, when I checked your inputs today, I noticed such errors are not throwing from the react-app. Upon inspection, I found out that, we were not including hooks rules in eslint. I will fix that issue and there after we will get beautiful (not sure i can call those irritating red lines "beautiful") warnings if we miss out any dependencies.