Discussion on: React race condition bug

View post

Replies for: Not sure i follow, can you post a code snippet?

Something like:

useEffect(() => {
  let selectedPet = pets.selectedPet;
  if (selectedPet) {
    dispatch({ type: "FETCH_PET" });
    getPet(selectedPet).then(data => {
      dispatch({ type: "FETCH_PET_SUCCESS", selectedPet, payload: data });
    });
  } else {
    dispatch({ type: "RESET" });
  }
}, [pets.selectedPet])

and

    case "FETCH_PET_SUCCESS": {
      if (state.selectedPet != action.selectedPet) {
        // out-of-order event, ignore
        return state;
      }
      return {
        ...state,
        loading: false,
        petData: action.payload
      };
    }

Sagiv ben giat • Oct 27 '19

This way the UI won't update untill we get back the data, which is fine for the info but may get a poor user experience for the drop down.
All in all this is just a verry simple example to show a possible bug, obviously the code could be a lot better, like validations etc 🙂

Thomas Broyer • Oct 27 '19

Hmm, did you try it? codesandbox.io/s/new-e6gpr

Here's the diff:

  useEffect(() => {
-   if (pets.selectedPet) {
+   let selectedPet = pets.selectedPet;
+   if (selectedPet) {
      dispatch({ type: "FETCH_PET" });
-     getPet(pets.selectedPet).then(data => {
-       dispatch({ type: "FETCH_PET_SUCCESS", payload: data });
+     getPet(selectedPet).then(data => {
+       dispatch({ type: "FETCH_PET_SUCCESS", payload: data, selectedPet });
      });
    } else {
      dispatch({ type: "RESET" });
    }
  }, [pets.selectedPet])

and

    case "FETCH_PET_SUCCESS": {
+     if (state.selectedPet != action.selectedPet) {
+       // out-of-order event, ignore.
+       return state;
+     }
      return {
        ...state,
        loading: false,
        petData: action.payload
      };
    }

Sagiv ben giat • Oct 27 '19

Aw you kept the onChange handler. I thought you meant to omit it and instead "listen" to the "FETCH_PET_SUCCESS" in order to update the drop-down.

Thomas Broyer • Oct 27 '19

Fwiw, to me, this feels cleaner than somehow duplicating the selectedPet state into a useRef.

Also, while the local selectedPet variable I used is not actually needed, I feel it reads better, but YMMV, using pets.selectedPet everywhere would work just as well. First diff would be reduced to:

  useEffect(() => {
    if (pets.selectedPet) {
      dispatch({ type: "FETCH_PET" });
      getPet(pets.selectedPet).then(data => {
-       dispatch({ type: "FETCH_PET_SUCCESS", payload: data });
+       dispatch({ type: "FETCH_PET_SUCCESS", payload: data, selectedPet: pets.selectedPet });
      });
    } else {
      dispatch({ type: "RESET" });
    }
  }, [pets.selectedPet])