Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React useEffect / useState not recognizing state values when working within a GraphQL subscription #14042

Closed
dabit3 opened this issue Oct 30, 2018 · 6 comments

Comments

@dabit3
Copy link

dabit3 commented Oct 30, 2018

Do you want to request a feature or report a bug?
bug? (maybe) or I could be using the hook wrong

What is the current behavior?
When using useEffect, the current state value is not available in the state, only the old state value is available.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

https://codesandbox.io/s/5xwjrjwo74

What is the expected behavior?
State is available in memory

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.7.0-alpha.0

code.zip

@AyWa
Copy link

AyWa commented Oct 31, 2018

I experienced that too, and I think it is little confusing at first but I think it is on purpose.

When you put the second argument to [] you basically said, that you wanted to run only on mount / unmount

Passing in an empty array [] of inputs tells React that your effect doesn’t depend on any values from the component, so that effect would run only on mount and unmount, never on updates.

However in your case you depend of the todoList state so you should put [todoList] like:

  useEffect(() => {
    const subscriber = API.graphql(graphqlOperation(subscription)).subscribe({
      next: data => {
        const {
          value: {
            data: { onCreateTodo }
          }
        } = data;
        console.log("todoList: ", todoList);
        updateTodos([...todoList, onCreateTodo]);
      }
    });
    return () => subscriber.unsubscribe();
  }, [todoList]); <- here ?

However I think it is not the proper way to do it (because you dont want to subscribe at every todoList change)

@thchia
Copy link

thchia commented Oct 31, 2018

[EDIT]: I take back the below comment, because adding [todoList] as the second argument to the subscriber does indeed fixe the issue. Maybe we do have to subscribe/unsubscribe on every render...


@AyWa I don't think that is the issue (although I see where you're coming from) - we would not want to re-subscribe if the list of todos changed. The subscription should be set up and torn down only once in the lifecycle. I tried moving the todoList variable out of the subscribe effect in case it was being referenced as a stale value, but the result is the same:

export function useSubscription() {
  const [todoList, updateTodos] = useState([]);

  // Removes the reference to `todoList` out of the effect, but state is still being reset somewhere
  function addTodosToState(newTodos) {
    updateTodos([ ...todoList, ...newTodos ])
  }

  useEffect(async () => {
    const todoData = await API.graphql(graphqlOperation(query));
    addTodosToState(todoData.data.listTodos.items);
  }, []);

  useEffect(() => {
    const subscriber = API.graphql(graphqlOperation(subscription)).subscribe({
      next: data => {
        const {
          value: {
            data: { onCreateTodo }
          }
        } = data;
        addTodosToState([onCreateTodo]); // Don't reference `todoList` in this effect
      }
    });
    return () => subscriber.unsubscribe();
  }, []);

  return todoList;
}

As far as I can tell the posted example follows the subscription example in the docs, with the exception that it is merging previous state.

@AyWa
Copy link

AyWa commented Oct 31, 2018

As far as I can tell the posted example follows the subscription example in the docs, with the exception that it is merging previous state.

It does not. In you example you have added [] in second parameter.

  useEffect(() => {
    ChatAPI.subscribeToFriendStatus(props.friend.id, handleStatusChange);
    // Specify how to clean up after this effect:
    return function cleanup() {
      ChatAPI.unsubscribeFromFriendStatus(props.friend.id, handleStatusChange);
    };
  });

Has you can see in the doc they don't put second argument, which means, that theeffect will be call everytimes the component rerender.

From the doc:

If you use this optimization, make sure the array includes any values from the outer scope that change over time and that are used by the effect

@AyWa
Copy link

AyWa commented Oct 31, 2018

Maybe we do have to subscribe/unsubscribe on every render...

@thchia I think there is multiple way to achieve that. One way is with reducer, but I guess there is other way too ? https://codesandbox.io/s/03l8z5m8nw

const initialState = { todoList: [] };

function reducer(state, action) {
  switch (action.type) {
    case "set":
      return { todoList: action.payload };
    case "add":
      return { todoList: [...state.todoList, action.payload] };
  }
}

export function useSubscription() {
  const [state, dispatch] = useReducer(reducer, initialState);

  useEffect(async () => {
    const todoData = await API.graphql(graphqlOperation(query));
    dispatch({ type: "set", payload: todoData.data.listTodos.items });
  }, []);

  useEffect(() => {
    const subscriber = API.graphql(graphqlOperation(subscription)).subscribe({
      next: data => {
        const {
          value: {
            data: { onCreateTodo }
          }
        } = data;
        dispatch({ type: "add", payload: onCreateTodo });
      }
    });
    return () => subscriber.unsubscribe();
  }, []);

  return state.todoList;
}

@dabit3
Copy link
Author

dabit3 commented Oct 31, 2018

Hey @AyWa this is the solution I think I was looking for. This does indeed fix both the issues I've been having (either subscriber being called on every render or the state being stale).

Thanks so much for your help with this, I'm going to go ahead and close this ticket!

@ayal
Copy link

ayal commented Jan 23, 2020

@AyWa 's solution worked for me (i.e using a reducer)

However I was able to solve a similar issue (subscribing once in useEffect and updating state in callback) while still using useState(), alongside with the "function updater" option of the state update function

as mentioned here:
https://reactjs.org/docs/hooks-reference.html#functional-updates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants