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

improve/extend async state for useAsync and related hooks #88

Open
xialvjun opened this issue Jan 2, 2019 · 16 comments
Open

improve/extend async state for useAsync and related hooks #88

xialvjun opened this issue Jan 2, 2019 · 16 comments
Labels
enhancement New feature or request

Comments

@xialvjun
Copy link

xialvjun commented Jan 2, 2019

just like Query Mutation in react-apollo:

<Query query={gql_document} variables={variables}>
  {({ data, error, loading }) => <div></div>}
</Query>

<Mutation mutation={gql_document}>
  {(mutation_api, mutation_result) => {
    const { data, error, loading } = mutation_result;
    return <div></div>
  }}
</Mutation>

And now we have useAsync, but it can only act as Query.

We should have an useApi to act as Mutation.

// A very very naive implementation
const useApi = (promisified_api, variables) => {
   const [state, set_state] = useState({loading: !!variables});
   const api = async (...variables) => {
    set_state({loading:true});
    const data = await promisified_api(...variables);
    set_state({data});
  }
  // we should expose `set_state`, it can act as Query's updteQuery, and use can set_state({error:null}) manually when close the error dialog.
  return [api, state, set_state];
}

const [api, state, set_state] = useApi(fn_return_promise, maybe_variables);
@benneq
Copy link
Contributor

benneq commented Jan 30, 2019

I'd say that there's no need for useApi, but instead the useAsync hook should get extended. It has some design flaws and is missing some features.

I've written my own useAsync version, which works like this:

const getData = (id: string) => {
    return getDataByIdViaAPI(id);
}

const MyComponent: React.FunctionComponent<Props> = (props) => {
    const { isLoading, isResult, isError, result, error, load } = useAsync(getData);

    useEffect(() => {
        load(props.id);
    }, []);

    return (
        ...
    );
}

It has additional isResult and isError properties, because it's possible to have a Promise which successfully resolves to null or undefined.

And it doesn't run your promise automatically. That's why there is the load parameter, which is a function, that will run your promise.

Maybe one should then add an loadOnMount option, so you don't have to write the useEffect hook, which only triggers the load function.

And maybe an isIdle property could be added, too. Don't know if it's necessary. So you can tell if the promise did already run.

Here's my implementation. There may be some optimizations left 😄

import { useState } from 'react';

export const useAsync = <T, ARGS extends any[]>(promise: (...args: ARGS) => Promise<T>) => {
    const [state, setState] = useState({
        isLoading: false,
        isResult: false,
        isError: false,
        result: undefined as undefined | T,
        error: undefined
    });

    const load = async (...args: ARGS) => {
        setState({ isLoading: true, isResult: false, isError: false, result: undefined, error: undefined });
        try {
            const result = await promise(...args);
            setState({ isLoading: false, isResult: true, isError: false, result: result, error: undefined });
        } catch(error) {
            setState({ isLoading: false, isResult: false, isError: true, result: undefined, error: error });
        }
    };

    return {...state, load: load};
};

@streamich
Copy link
Owner

@benneq what do you think about replacing isLoading, isError and isResult by simply a promise state, where

let state: 'peding' | 'error' | 'resolved';

@benneq
Copy link
Contributor

benneq commented Jan 30, 2019

That should be fine, too. I'm already a bit annoyed because there are so many props 😆

If you want to keep the Promise naming scheme, I'd say it should be called rejected instead of error. Here ( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise ) they call it pending | fulfilled | rejected. I'm not sure what's "best".

There are only two hard things in Computer Science: cache invalidation and naming things.

-- Phil Karlton

What do you think about idle? It's the state before the first isLoading. Or simply leave it as undefined?

@streamich
Copy link
Owner

What do you think about idle?

let state: 'idle' | 'pending' | 'rejected' | 'fulfilled';

😄

@benneq
Copy link
Contributor

benneq commented Jan 30, 2019

Sounds reasonable! 👍

And what about the load (or whatever you want to call it) property? So you can trigger the hook manually.

@streamich
Copy link
Owner

And what about the load...

That's interesting, but that's a breaking change. Maybe could be a flag to have load or do loading automatically.

@benneq
Copy link
Contributor

benneq commented Jan 30, 2019

Yes, there should be some option / flag to enable/disable this. My version has loadOnMount: false by default, but yours could have loadOnMount:true by default. Should be no problem.

useAsync(myPromise, { loadOnMount: false }); // the "options" object. I already another idea for an option ;)

@xialvjun
Copy link
Author

const useApi = (api_fn, variables) => {
  const [state, set_state] = useState({ loading: !!variables, error: null, res: null });
  const api = async (...variables) => {
    set_state({ loading: true, error: null, res: state.res });
    try {
      const res = await api_fn(...variables);
      set_state({ loading: false, error: null, res });
    } catch (error) {
      set_state({ loading: false, error, res: state.res });
      throw error;
    }
  };
  // if has variables, it loads at start even the variables is just an empty array []
  useEffect(_ => !!variables && api(...variables), [...(variables || [])]);
  return [api, state, set_state];
};

const data = new Array(100).fill(0).map(_ => Math.random());

const rest_api = n => new Promise(res => setTimeout(res, 1000)).then(_ => data.filter(d => d > n));

const MyComponent = props => {
  const [n, set_n] = useState(0);
  // useApi(rest_api, [n]) --- has variables, so it loads at start. If you just want an async api, useApi(rest_api)
  const [search, search_state, set_search_state] = useApi(rest_api, [n]);
  return (
    <div>
      <div>
        <label>
          number: <input value={n} onChange={e => set_n(parseFloat(e.target.value) || 0)} />
        </label>
        <button onClick={_ => search(n)}>research</button>
      </div>
      {search_state.loading && "loading..."}
      {search_state.error && <Dialog content={search_state.error} onClose={_ => {
        // you can manully set the state to close the error dialog, or use set_state as Apollo.Query.updateQuery
        set_search_state({ ...search_state, error: null })
      }} />}
      <ul>
        {(search_state.res || []).map(n => (
          <li key={n}>{n}</li>
        ))}
      </ul>
    </div>
  );
};

@benneq
Copy link
Contributor

benneq commented Jan 30, 2019

@xialvjun So the only real difference is the set_state stuff, right?

I'm not sure if this a common use case. But there might be a simple workaround:

const [search, search_state, _] = useApi(rest_api, [n]);

const [showError, setShowError] = useState(false);
useEffect(() => {
  if(search_state.error) {
    setShowError(true);
  }
}, [search_state.error]);

return (
    // ...
    {showError && <Dialog content={search_state.error} onClose={_ => setShowError(false)} />
    // ...
)

@benneq
Copy link
Contributor

benneq commented Jan 31, 2019

I found another possible issue that must be solved somehow: Concurrent requests. For example if you have some autocomplete / typeahead search box, that is powered by API.

const { value, load } = useAsync(...);

load(...); // first, response takes 100ms
load(...); // second, response takes 2000ms
load(...); // third, response takes 500ms

useAsync must ensure that the latest request (not response!) wins. In the example above, we should receive the responses for the first and the third load call. When the second one responds, it has to be ignored, because the third request already finished.

My implementation currently doesn't respect this case 😞 But I have two ideas:

  1. Use a simple counter, that increments for each request. And then ignore the response if currentCounter !== requestCounter. Or:
  2. Save the call arguments. And then ignore the response if currentCallArgs !== requestCallArgs. I'm not sure if this will always work.

Another (maybe useful?!) option would be to provide another "option", that prohibits additional load calls, while it's already loading. But this could also be solved by simply using something like this:

<button disabled={isLoading} onClick={() => load(...)}>Click me</button>

@benneq
Copy link
Contributor

benneq commented Jan 31, 2019

And I also found a use case for setState:

I have a todo list, which uses useAsync to fetch the list (sorted by dueDate).
On top of the list is an <AddTodo /> component. When adding a new todo, it should appear on top of the list. The dueDate doesn't matter. It must always the on top.
So I can't simply call load, because then, the new todo will be somewhere else in the list (depending on its dueDate).

Though it should be possible to call something like setState({ ...state, value: [newTodo, ...value] });

This could be worked-around by using a separate list for newTodos, which will always be displayed at top.

But... Additionally you can setCompleted on each todo. To workaround this, I would have to look into both lists (newTodos, and fetchedTodos) to find the matching one. This should work too, but it's getting ugly 😆

setState would be much easier.


EDIT: workaround isn't that easy, because I've got to trigger a state change within the component after running setCompleted.

setState is the way to go!

@streamich
Copy link
Owner

When calling load() multiple times there is another issue that you might already have a previous result, then you call load() and state would be set to pending, though you already have a previous result that you can display, but the next result is loading, so your component must be able to handle that case.

@benneq
Copy link
Contributor

benneq commented Jan 31, 2019

I'd say: It's a feature! 😄 Depending on your UX Styleguide, you can do some cool stuff with that. Of course you have to take care of it in your component.

For example:

  • Completely hide the results, while state != fulfilled. Just show a loading message / indicator
  • Or you could still display the old results with a semi-transparent non-clickable overlay, while state != fulfilled. And then exchange the contents when the response arrived.
  • Or you can hide / disable the load button, while state == pending. Prevent additional requests, while it's running. Or keep the button enabled and use some simple code like if(state != pending) load();

All I see there is: Having a lot of possibilities!


In my current test project, there's a text box, where you can enter your search term (using your useDebounce hook 😄 ). This will then trigger useEffect, and this will trigger load(searchTerm). (Before I used PureComponent, which was quite painful, +50 lines of code, +1500 bytes of minified code). And now look it looks like this:

const { isLoading, result, load } = useAsync(getTasks);

const [filter, setFilter] = useState({});
useEffect(() => load(filter), [filter]);

const [searchTerm, setSearchTerm] = useState('');
useDebounce(() => setFilter({ ...filter, searchTerm: searchTerm }), 300, [searchTerm]);

return (
  <>
    <input value={searchTerm} onChange={e => setSearchTerm(e.target.value)} />
    // display results / loading indicator
  </>
)

The only problem is when you have inconsistent slow internet connection: Then it's possible that you execute load multiple times, but it will display the wrong result, because request 2 took longer than request 3.

@benneq
Copy link
Contributor

benneq commented Jan 31, 2019

Another use case: Load more / infinite scroll. How would you handle this?
If this works without setState, then maybe we don't need setState at all. 😄 Because if there's another way of modifying the displayed results, then we could (means: should) keep useAsync as simple as possible.

I haven't tried it yet... but, is it possible to "concat" using hooks?

Pseudocode / untested / fantasy:

const { result, load } = useAsync(...);

const [combinedResults, setCombinedResults] = useState([]);

useEffect(() => {
  if(result) { // will only trigger if we get new results ... hopefully?
    setCombinedResults(...combinedResults, ...result);
  }
}, [result]);

const loadNextPage = () => {
  load(currentPage + 1);
};

And here we have a use case, where parallel / concurrent load is forbidden. Because else we could end up with combinedResults like this: [page1, page2, page2, page2, page4, page3]. But maybe this could be simply done by adding if state != pending inside the loadNextPage function.


What we (means: you! 🤣 ) should keep in mind: Hooks are very easily composable. In other words: If it's possible to create a custom useAsyncWithSetState hook (that uses useAsync internally), then setState shouldn't be part of useAsync, I'd say.

@dfee
Copy link

dfee commented Feb 23, 2019

Hey guys, I used useAsync as a starting point, and grappled with the problem that I didn't necessarily want to run it on first load...

So I came up with the following re-implementation: https://gist.github.com/dfee/0686746c4b37ad45ba88b20a66a60432

and here's some example usage (I'm passing onSubmit to a Formik form...):

  const [state, utils] = useAsync<WrappedFormUtils>(auth.signInWithEmail, {
    onFinally: ({ context }) => {
      if (context !== undefined) {
        context.resetFields(["password"]);
      }
    },
  });

  const onSubmit: SignInFormProps["onSubmit"] = (e, form) => {
    utils.setArgs([form.getFieldsValue()]);
    utils.setContext(form);
  };

I'm sharing this here because I think my solution is effective, but might be improved by the community, and perhaps merged in to replace the current useAsync implementation.

@wardoost wardoost added the enhancement New feature or request label Mar 31, 2019
@wardoost wardoost changed the title useApi ? useApi Mar 31, 2019
@wardoost wardoost changed the title useApi improve/extend async state for useAsync, useAsyncFn and useAsyncRetry Oct 5, 2019
@wardoost wardoost changed the title improve/extend async state for useAsync, useAsyncFn and useAsyncRetry improve/extend async state for useAsync and related hooks Oct 5, 2019
@martineso
Copy link

So I have tackled this problem. I am not sure whether it is feasible to extend the useAsync hook or provide another one that has an API to [state, setState].
I had an array of data I had to fetch from an API and then I had to remove add or update items locally in the React app instead of just calling the API again. This is appeared to be a problem with the original implementation of the useAsync hook since it does not provide an api to the modify the state of the data return from the async fn.
I have made some modifications to the useAsyncFn hook as it was easier. (useAsync hook just calls useAsyncFn hook).

Below is the difference in the API that useAsync hook exposes using a trivial todo example:

  • Original version
const {
        loading,
        error,
        value
    } = useAsync(async () => {
           const result = await apiClient.get('/todos');
           return result;
    }, []);
// todos cannot be modified locally
  • Modified version
const {
        loading,
        error,
        data: [todos, setTodos]
    } = useAsync(async () => {
        const result = await apiClient.get('/todos');
        return result;
    }, []);

// This is will work with the local state of todos
setTodos(todos.filter(todo => todo.type !== 'done'))

I can do a pull request but I haven't really read through the contributing guidelines and Typescript can be a problem since I have not used it before. Let me know what you think.

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

No branches or pull requests

6 participants