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

Async useEffect is pretty much unreadable #14326

Closed
Tracked by #10
linvain opened this issue Nov 26, 2018 · 8 comments
Closed
Tracked by #10

Async useEffect is pretty much unreadable #14326

linvain opened this issue Nov 26, 2018 · 8 comments

Comments

@linvain
Copy link

linvain commented Nov 26, 2018

heh

The only alternative is using then() chains which aren't very readable at their own.

@linvain linvain changed the title Asyns useEffect is pretty much unreadable Async useEffect is pretty much unreadable Nov 26, 2018
@gaearon
Copy link
Collaborator

gaearon commented Nov 26, 2018

I think you're making it more complicated than it needs to be.

useEffect(() => {
  async function fetchMyAPI() {
    let url = 'http://something';
    let config = {};
    const response = await myFetch(url);
    console.log(response);
  }  

  fetchMyAPI();
}, []);

If it uses some prop like productId:

useEffect(() => {
  async function fetchMyAPI() {
    let url = 'http://something/' + productId;
    let config = {};
    const response = await myFetch(url);
    console.log(response);
  }  

  fetchMyAPI();
}, [productId]);

However, note that this doesn't guarantee requests come in order. So a better solution would be:

useEffect(() => {
  let didCancel = false;

  async function fetchMyAPI() {
    let url = 'http://something/' + productId;
    let config = {};
    const response = await myFetch(url);
    if (!didCancel) { // Ignore if we started fetching something else
      console.log(response);
    }
  }  

  fetchMyAPI();
  return () => { didCancel = true; }; // Remember if we start fetching something else
}, [productId]);

This is a good article that goes into more detail and has more useful examples: https://www.robinwieruch.de/react-hooks-fetch-data/

In longer term, we'll recommend Suspense for data fetching which will look more like

const response = MyAPIResource.read();

and no effects. But in the meantime the code above works fine.

@lxe
Copy link

lxe commented Dec 16, 2018

What if I need to close over setSomething(value returned from async)?

// Elsewhere
async function fetchWidgets() {
  return await fetch('./api/widgets').then(r => r.json())
}

// In my component:
function MyComponent() {
  const [widgets, setWidgets] = useState([]);
  // Need another async wrapper function to close over `setWidgets`?
  useEffect(() => (async () => setWidgets(await fetchWidgets()))());

(Or based on your example):

function MyComponent() {
  const [widgets, setWidgets] = useState([]);
  async function unnecessaryApiWrapperClosureThatAddsLayersOfIndirection() {
     setWidgets(await fetchWidgets());
  }
  useEffect(() => unnecessaryApiWrapperClosureThatAddsLayersOfIndirection());

Also where do the errors go that are thrown in that async function invocation? I'd need to .catch() it? Or try/catch the body of the async so the promise always resolves?

useEffect(() => (async () => { 
  try { 
    setWidgets(await fetchWidgets());
  catch e {
    setError(e);
  }
})());

Why not make useEffect take an async function if we'd have to wrap them all the time anyways?

@lxe
Copy link

lxe commented Dec 16, 2018

I guess one can always just then:

function MyComponent() {
  const [widgets, setWidgets] = useState([]);
  useEffect(() => { 
    fetchWidgets.then(w => setWidgets(w)) 
  });

@RiJung
Copy link

RiJung commented Feb 8, 2019

@gaearon
now I should use useEffect instead of componentDidMount into a React Component with Hooks.

Scenario "initial unique call to a server":
To accomplished this, DependencyList ( second argument of useEffect) in useEffect should every time an empty array otherwise the application will send every state change a fetch call to the server.

it' means , this it the best practise to getData

    useEffect(() => {
        console.log("useEffect, fetchData here");
    }, []);

Full Code here

import React, { useState, useEffect } from 'react';

export function CaseAssignDropDown() {
    const [count, setCount] = useState(0);
    useEffect(() => {
        console.log("useEffect, fetchData here");
    }, []);
    /*
    [] -> get data once
    [count] or removed [] -> get data each click
    */
    return (
        <div>
            <p>You clicked {count} times</p>
            <button onClick={() => setCount(count + 1)}>
                Click me
                </button>
        </div>
    );
}

https://stackoverflow.com/questions/54596192/react-hooks-async-best-practise

@gaearon
Copy link
Collaborator

gaearon commented Feb 8, 2019

Please ask on StackOverflow and feel free to link to your question from here. We don’t use issues for support because answers get lost on GitHub.

@codeaid
Copy link

codeaid commented Feb 22, 2019

componentDidMount supports being async so I think useEffect should accept a promise and automatically resolve it to avoid having to .then them.

@gaearon
Copy link
Collaborator

gaearon commented Feb 22, 2019

@codeaid It is an intentional design decision. The async/await model is a pitfall for effects in general (and has always been a pitfall in classes!) due to race conditions. In the longer term, Suspense will be the recommended data fetching solution, and you can do async / await there. But Suspense for data fetching is not ready yet.

@gaearon
Copy link
Collaborator

gaearon commented Mar 12, 2019

@lxe

As I explained above, setState(await something())) is a bad idea because you don't know whether await something() is still relevant. For example, if the user starts fetching one profile page but then switches to another profile. Then you're at the mercy of which request arrives first, and can show incorrect data.

This kind of code would handle it correctly:

useEffect(() => {
  let didCancel = false;

  async function fetchMyAPI() {
    let url = 'http://something/' + productId;
    let config = {};
    const response = await myFetch(url);
    if (!didCancel) { // Ignore if we started fetching something else
      console.log(response);
    }
  }  

  fetchMyAPI();
  return () => { didCancel = true; }; // Remember if we start fetching something else
}, [productId]);

I'm going to lock this issue as resolved to prevent further confusion. Here's a good article that answers your other questions (e.g. about loading states): https://www.robinwieruch.de/react-hooks-fetch-data/

@facebook facebook locked as resolved and limited conversation to collaborators Mar 12, 2019
katmdev referenced this issue in katmdev/the-shoppies Sep 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants