8

I have a list of objects ("Albums" in my case) fetched from the database. I need to edit these objects. In the editing component in the useEffect hook I fire up the action for getting the needed album using it's ID. This action works. However in the same useEffect I am trying to fetch the changed by before fired action redux state. And now I face the problem - all I am fetching is the previos state. How can I implement in the useEffect fetching of current redux state?

I've seen similar questions here, however none of the answers were helpfull for my use case.

I am using redux-thunk.

Editing component. The problem appears in setFormData - it's fetching previous state from the reducer, not the current one. It seems that it fires before the state gets changed by the getAlbumById:

//imports

const EditAlbum = ({
  album: { album, loading},
  createAlbum,
  getAlbumById,
  history,
  match
}) => {
  const [formData, setFormData] = useState({
    albumID: null,
    albumName: ''
  });

  useEffect(() => {
    getAlbumById(match.params.id);

    setFormData({
      albumID: loading || !album.albumID ? '' : album.albumID,
      albumName: loading || !album.albumName ? '' : album.albumName
    });

  }, [getAlbumById, loading]);

const { albumName, albumID } = formData;

  const onChange = e =>
    setFormData({ ...formData, [e.target.name]: e.target.value });

  const onSubmit = e => {
    e.preventDefault();
    createAlbum(formData, history, true);
  };

  return ( //code );
};



EditAlbum.propTypes = {
  createAlbum: PropTypes.func.isRequired,
  getAlbumById: PropTypes.func.isRequired,
  album: PropTypes.object.isRequired
};

const mapStateToProps = state => ({
  album: state.album
});

export default connect(
  mapStateToProps,
  { createAlbum, getAlbumById }
)(withRouter(EditAlbum));

Action:

export const getAlbumById = albumID => async dispatch => {
  try {
    const res = await axios.get(`/api/album/${albumID}`);

    dispatch({
      type: GET_ALBUM,
      payload: res.data
    });
  } catch (err) {
    dispatch({
      type: ALBUMS_ERROR,
      payload: { msg: err.response.statusText, status: err.response.status }
    });
  }
};

reducer

const initialState = {
  album: null,
  albums: [],
  loading: true,
  error: {}
};

const album = (state = initialState, action) => {
  const { type, payload } = action;
  switch (type) {
    case GET_ALBUM:
      return {
        ...state,
        album: payload,
        loading: false
      };
    case ALBUMS_ERROR:
      return {
        ...state,
        error: payload,
        loading: false
      };
    default:
      return state;
  }
};

Will be grateful for any help/ideas

ysvet
  • 163
  • 1
  • 1
  • 10

1 Answers1

8

You should split up your effects in 2, one to load album when album id changes from route:

const [formData, setFormData] = useState({
    albumID: match.params.id,
    albumName: '',
});
const { albumName, albumID } = formData;

// Only get album by id when id changed
useEffect(() => {
    getAlbumById(albumID);
}, [albumID, getAlbumById]);

And one when data has arrived to set the formData state:

// Custom hook to check if component is mounted
// This needs to be imported in your component
// https://github.com/jmlweb/isMounted

const useIsMounted = () => {
  const isMounted = useRef(false);
  useEffect(() => {
    isMounted.current = true;
    return () => (isMounted.current = false);
  }, []);
  return isMounted;
};

// In your component check if it's mounted 
// ...because you cannot set state on unmounted component
const isMounted = useIsMounted();
useEffect(() => {
  // Only if loading is false and still mounted
  if (loading === false && isMounted.current) {
    const { albumID, albumName } = album;
    setFormData({
      albumID,
      albumName,
    });
  }
}, [album, isMounted, loading]);

Your action should set loading to true when it starts getting an album:

export const getAlbumById = albumID => async dispatch => {
  try {
    // Here you should dispatch an action that would
    //  set loading to true
    // dispatch({type:'LOAD_ALBUM'})
    const res = await axios.get(`/api/album/${albumID}`);

    dispatch({
      type: GET_ALBUM,
      payload: res.data
    });
  } catch (err) {
    dispatch({
      type: ALBUMS_ERROR,
      payload: { msg: err.response.statusText, status: err.response.status }
    });
  }
};

Update detecting why useEffect is called when it should not:

Could you update the question with the output of this?

//only get album by id when id changed
useEffect(() => {
  console.log('In the get data effect');
  getAlbumById(albumID);
  return () => {
    console.log('Clean up get data effect');
    if (albumID !== pref.current.albumID) {
      console.log(
        'XXXX album ID changed:',
        pref.current.albumID,
        albumID
      );
    }
    if (getAlbumById !== pref.current.getAlbumById) {
      console.log(
        'XXX getAlbumById changed',
        pref.current.getAlbumById,
        getAlbumById
      );
    }
  };
}, [albumID, getAlbumById]);
Kaspar Lee
  • 5,446
  • 4
  • 31
  • 54
HMR
  • 37,593
  • 24
  • 91
  • 160
  • It did solved the problem, thanks for the very detailed explanations. Got into a loop, but left the brackets empty in the first useEffect - worked good as well. – ysvet Sep 26 '19 at 09:58
  • Ok, found another problem - i do get now current data, but not on the first call. On the first call the album in the reducer state still is null (getting the TypeError: Cannot read property 'albumID' of null in the second useEffect). Refreshing the editing page leads to getting the needed album. – ysvet Sep 26 '19 at 10:15
  • Additional check in the second useEffect for if the album is not null solved the issue of the first call – ysvet Sep 26 '19 at 10:57
  • @ysvet The first useEffect should not keep calling itself unless you do an unnecessary unmount and remount. I assume that albumId is a primitive value that comes from the router and getAlbumById is the same reference to an action creator function that is created by connect. – HMR Sep 26 '19 at 11:00
  • @ysvet Album cannot be null and loading false at the same time. Unless you have an error but then you shouldn't try to display the album anyway. – HMR Sep 26 '19 at 11:00
  • Yes, albumID is a primitive and comes from the router and getAlbumById is the same reference to an action creator function that is created by connect. However when I add them as effect dependencies it seems that the useEffect starting calling itself for apparantely a random number of times (well, I am pretty sure that is nothing random here, however I haven't found any cause of this behavior yet). Without effect dependencies all the actions are getting fired just once. – ysvet Sep 26 '19 at 12:16
  • @ysvet You could leave out `getAlbumById` it'll give you a linting error but it'll give you an idea of what is changing. You can return a cleanup function from the effect and see when it's called. Also log the albumId and see if that doesn't change. They are not suppose to change so if they do you may have a bug somewhere else. – HMR Sep 26 '19 at 13:48
  • @ysvet I updated my anser, at the bottom is the useEffect to get the album with logs so you can see what's going on. – HMR Sep 26 '19 at 13:57
  • I am not sure what pref is in pref.current.albumID. Could you please explain it so I could define it somehow to run the code? – ysvet Sep 26 '19 at 14:45
  • @ysvet sorry, forgot: just before the useEffect:`const pref = useRef({albumID,getAlbumById})` then you can see if these values change in useEffect every time the function is run by React. – HMR Sep 26 '19 at 20:23
  • I've been analyzing the logs. They showing some curios behavior indeed. So, first run of editing component goes ok. Second (go back and edit another album) - goes into small loop - the new albumID keeps switching with the previous until it stays with the new one. Third attempt (go back to the album list and edit the third album) - another loop, and the third ID is swithching not with previous, but with first one. This loop is longer (about 100 times or so) – ysvet Sep 27 '19 at 07:16
  • All the loops eventually are stoping on the needed albumID, and in all cases the looping is the swithching between the called albumID and the very first called albumID. – ysvet Sep 27 '19 at 07:27
  • @ysvet maybe you add an event listener on something that will open the edit screen, you didn't remove the event listener but keep adding it so the first time you click it opens the screen once, the second time twice, the third time three times ... Maybe you can inspect the code that opens the album edit window and log in handler to open it. – HMR Sep 27 '19 at 07:50
  • The page with album list from where I open the editing is pretty simple and straitforward - it does just fetching from the db and displaying the list. No event listeners, just react-router Links. I do use an useEffect there for fetching the album list, but with effect dependencies. ` useEffect(() => { getAllAlbums(); }, [getAllAlbums]);` Anyway many thanks for your help, your suggestions are really helping me a lot. – ysvet Sep 27 '19 at 08:04
  • As for the login - in the redux DevTools I can see that all the actions with login and user loading are firing just once in the very begining, nothing unusual here. Well, anyway with your help the code now really does what it's expected from it. I promise to dive deeper in the React Hooks, it's still something new for me. – ysvet Sep 27 '19 at 08:16