Use React props as a function argument

Question

I want to check if this is good practice, and if there's a better approach to it. I have a function that is called when the component loads and when clicking on a couple of buttons, so I need to use it inside useEffect, and pass it to other components. I could use useCallback, but I don't see how this approach is not enough, since the components that can call getWeatherHere also need isCelsius, thus being able to use it as argument (the argument and the state are the same).

export default function weatherInfo() {
  const [weatherInfo, setWeatherInfo]: [IWeather, React.Dispatch<IWeather>] = useState();
  const [isCelsius, setIsCelsius] = useState(true);

  function getWeatherHere(isCelsius: boolean) {
    navigator.geolocation.getCurrentPosition(async ({coords: {latitude, longitude}}) => {
      const data = await Weather.getWeather(latitude, longitude, !isCelsius ? 'imperial' : undefined);
      setWeatherInfo(data);
    });
  }

  useEffect(() => {
    getWeatherHere(isCelsius);
  }, [isCelsius]);

  return (
    <OtherComponent isCelsius={isCelsius} getWeatherHere={getWeatherHere}/>
  );
}

If you are calling the function in the useEffect, isCelsius will update on the state whenever the function is called, so there is no need to pass state Varaible as params, you can simply use isCelesius from state inside the function.. — sms, Sep 05 '22 at 14:15
Yeah, that makes sense, i'm getting an exhaustive-deps warning about getWeatherHere, but i guess that's not always right — Guido Glielmi, Sep 05 '22 at 14:25

Halcyon · Answer 1 · 2022-09-05T14:24:44.417

This is how you do it, but there are tools available to make this easier.

Consider using a construction like useAsync from react-use. It allows you to express a Promise as a state. This avoids all sort of pitfalls and issues of using Promises in functional Components.

So you get something like:

const state = useAsync(async () => {
    const { coords: { latitude, longitude } } = await navigator.geolocation.getCurrentPosition();
    return {
        isCelsius: isCelsius,
        data: await Weather.getWeather(latitude, longitude, !isCelsius ? 'imperial' : undefined),
    };
}, [ isCelsius ]);

return <>
    {state.loading === false && <OtherComponent 
        isCelsius={state.value.isCelsius}
        weatherHere={state.value.data}
    />
</>;

Some remarks:

I added isCelsius to the value of the async state. If you don't do that, and pass isCelsius directly, you're going to have a desynch between the value of isCelsius and the weather data. I would actually expect that the temperature unit is part of the result of the getWeather request, but if it's not, this works.
There is one problem when using promises with setState in React, and that has to do with cleanups. If your Component is unmounted before the promise is completed, it doesn't magically cancel the code. It will finish the request and call setWeatherInfo. That will trigger a re-render on an unmounted component, which React will give you a warning about. It's not a huge problem in this case, but it can become a problem in more complex situations. useAsync takes care of this by checking if the component is still mounted at the end of the fetcher function. You can also do that yourself by using usePromise, but I would try to avoid using Promises in this way all together.
Your code can suffer from race conditions. If you change isCelsius quickly a couple of times, it's going to be a coinflip which result is going to end up being used. useAsync also takes care of this.

If, instead of passing the weather, you want to pass a function that fetches the weather, use useAsyncFn instead. The state is the same, but it also returns a function that allows you to call the fetcher function. This is in addition to the value of isCelsius changing.

score 0 · Answer 2 · answered Sep 05 '22 at 15:08

As your post is about best practices, I'll let you my 2 cents. Some things that I would change using only pure react to refactor it.

export default function weatherInfo() {
  // You don't need to type both returns, only the hook.
  const [weatherInfo, setWeatherInfo] = useState<IWeather | undefined>(undefined);

  // Why do you need this state if it's not changing? 
  // May you can put this in a const and save one state.
  const isCelsius = true;

  // I may change this "Here" in the function, because it don't look wrong, by it's subjective
  // Use "ByUserLocation" it's a better description
  // With the hook useCallback you can rerender the function only based on the right values
  const getWeatherByUserLocation = useCallback(() => {
    // Extracting the function improves the maintenance by splitting each step of the process and each callback function.
    const callbackFn = async ({coords: {latitude, longitude}}) => {
      // Turn your validations into variables with a well-descriptive name.
      // Avoid making validation starting by negating the value;
      const temperatureUnit = isCelsius ? 'celsius' : 'imperial'; 

      // Avoid using "data" as the name of a variable even when it looks clear what the data is. In the future, you will not remember if this logic should change.
      const weatherInfoData = await Weather.getWeather(
        latitude, 
        longitude, 
        temperatureUnit
      );

      setWeatherInfo(weatherInfoData);
    };

    navigator.geolocation.getCurrentPosition(callbackFn);
  }, [isCelsius]);

  useEffect(() => {
    getWeatherByUserLocation();
    // removing the prop from the function, you avoid calling it many times based on the state change
    // A useEffect without dependencies on the dependency array, it will only be called on the component mount and unmount
  }, []);

  return (
    <OtherComponent isCelsius={isCelsius} getWeatherByUserLocation={getWeatherByUserLocation}/>
  );
}

Use React props as a function argument

2 Answers2