reactjs / rfcs

RFCs for changes to React
MIT License
5.5k stars 557 forks source link

useEffect goes in inifinite loop with React Hooks #113

Closed m-nathani closed 5 years ago

m-nathani commented 5 years ago

React/Redux application goes into an infinite loop on using useEffect with object references..

I am trying render pending todos for my application using useEffect.. and passing the array of todos as the second param in useEffect ..but why is not checking the values of the object ?

Container:

const mapDispatchToProps = dispatch => ({ actions: bindActionCreators(RootActions, dispatch) });
const Home = (props) => {
  const { root, actions } = props;

  useEffect(() => {
    getTodos(actions.loadPendingTodo);
  }, [root.data]);

  return (
    <Segment>
      <Error {...root } />
      <TodoList { ...root } actions={actions} />
  </Segment>
  );
};

export default connect(mapStateToProps, mapDispatchToProps)(Home);

Action:

export const loadPendingTodo = () => ({
  type: LOAD_PENDING_TODO,
  data: todoService.loadPendingTodo(),
});

Reducer:

const initialState = {
  initial: true,
  data: [{
    id: 0,
    text: 'temp todo',
    dueDate: new Date(),
    completedDate: '',
    isDeleted: false,
    isCompleted: false,
  }],
  error: false,
  isLoading: false,
  isEdit: false,
};

export default function root(state = initialState, action) {
  switch (action.type) {
    case LOAD_PENDING_TODO:
      return {
        ...state,
        data: [...action.data],
      };
...

    default:
      return state;
  }
}

getTodos Method:

export const getTodos = (loadTodo) => {
  try {
    loadTodo();
  } catch (error) {
    console.log(error); // eslint-disable-line
  }
};

Service:

export default class TodoAppService {
  loadPendingTodo() {
    return store.get('todoApp').data.filter(todo => !todo.isCompleted && !todo.isDeleted);
  }

Can anyone please help me out how to resolve this issue.. and there is no official documentation for this case too :/

Moreover changing the useEffect to the following works but i want to render on every change

  useEffect(() => {
    getTodos(actions.loadPendingTodo);
  }, []);
phryneas commented 5 years ago

Here

  useEffect(() => {
    getTodos(actions.loadPendingTodo);
  }, [root.data]);

you're telling React "everytime, root.data is a reference to a new object, execute getTodos(actions.loadPendingTodo)" (which basically executes actions.loadPendingTodo and causes an action to be thrown into the reducer)

Within your reducer, you create a new array with the contents you need.

This new array is a new object, meaning root.data is a different object reference as before, triggering useEffect again.

As another way to view it: even if the arrays have the same content, they are different arrays. Just like putting the same content from one cardboard box to another does not make the two cardboard boxes one and the same box.

What you could do here, is to in your reducer shallowCompare action.data with state.data and only create a new array if the contents actually changed.

phryneas commented 5 years ago

Oh, and one more thing: this isn't really the FAQ section of React here - in the future, something like stackOverflow might be a better place to ask questions ;)

m-nathani commented 5 years ago

Fixed it by removing the loadPedningTodo redux actions in useEffect that was causing it to loop and directly setting the data in function from service..

const Home = (props) => {
  const { root, actions } = props;
  return (
    <Segment>
      <Error {...root } />
      <TodoList isEdit={root.isEdit} todo={todoService.loadPendingTodo()} actions={actions} />
  </Segment>
  );
};

thanks :)

phryneas commented 5 years ago

I didn't even notice that in the first place. You should never ever directly access your redux store from within a React component - that's what you use the react-redux library for. Even with hooks, you will still have to stick to connect() calls for now - in the near future there will be a useSelector or similar hook you can use. But for now, there is nothing like that officially.

If you need a general refresher on redux, check here: https://egghead.io/courses/getting-started-with-redux

Edit: but as I wrote, this is not really the place for that - with every comment we write here, most likely hundreds of people are getting notification emails... so, please, direct any further questions to stackoverflow :)

leandroaps commented 5 years ago

Try something like:

useEffect(() => { state.length === 0 && fetch(dispatch); }, [dispatch, state]);