rauldeheer / use-async-effect Goto Github PK

I cannot use npm at all because of this package. I get this error:

npm ERR! path /Users/admin/project/node_modules/use-async-effect
npm ERR! code EISGIT
npm ERR! git /Users/admin/project/node_modules/use-async-effect: Appears to be a git repo or submodule.
npm ERR! git    /Users/admin/project/node_modules/use-async-effect
npm ERR! git Refusing to remove it. Update manually,
npm ERR! git or move it out of the way first.

Sure enough running ls -la ./node_modules/use-async-effect lists the following:

total 48
drwxr-xr-x    10 admin  staff    320 Jul  8 15:50 .
drwxr-xr-x  1228 admin  staff  39296 Jul  8 16:06 ..
-rw-r--r--     1 admin  staff     89 Oct 26  1985 .flowconfig
drwxr-xr-x     3 admin  staff     96 Jul  8 15:50 .git
-rw-r--r--     1 admin  staff   1078 Oct 26  1985 LICENSE
-rw-r--r--     1 admin  staff    711 Oct 26  1985 README.md
-rw-r--r--     1 admin  staff    358 Oct 26  1985 index.js
-rw-r--r--     1 admin  staff    248 Oct 26  1985 index.js.flow
-rw-r--r--     1 admin  staff   1756 Jul  8 15:50 package.json
drwxr-xr-x     5 admin  staff    160 Jul  8 15:50 types

Hey it's franciscop from this PR on my work account. While isMounted() works great for requests where you normally go forward/backwards between pages, I think this could also greatly benefit from an isLatest(), isFresh() or similar. For this code:

export default ({ userId }) => {
  const [user, setUser] = useState(false);

  useAsyncEffect(async isMounted => {
    const user = await fetch(`/users/${userId}`);
    if (!isMounted()) return;
    setUser(user);
  }, [userId]);

  return <div>{user ? user.name : "Loading..."}</div>
};

It'll not try to set the user on an unmounted component, which is already great and fixing so many issues. But, if the userId changes, the component will not remount so there's effectively a race condition and if the second fetch() finishes before the first one it'll overwrite the data. If we also had a isLatest (a superset of isMounted) this race condition could be avoided:

export default ({ userId }) => {
  const [user, setUser] = useState(false);

  useAsyncEffect(async (isMounted, isLatest) => {
    const user = await fetch(`/users/${userId}`);
    if (!isLatest()) return;
    setUser(user);
  }, [userId]);

  return <div>{user ? user.name : "Loading..."}</div>
};

If this feature is welcome, there are three options here:

• Add it as a new parameter: useAsyncEffect(async (isMounted, isLatest) => ...);. The problem is that isLatest() is a superset of isMounted(), so if you are using isLatest() then isMounted() is not needed.
• Release a major version with a new API and named parameters: useAsyncEffect(async ({ isMounted, isLatest }) => {...});
• Make it a bit hacky to keep both of the above options available. No need for major release, can work as a parameter. Both of these work: useAsyncEffect(async isMounted => ...) and useAsyncEffect(async ({ isMounted, isLatest }) => ...)

I like the last one slightly more, and you can remove the legacy way in the future when there's a major release. But it's up to what you prefer 😄

In the README it says:

The async callback will receive a single function to check whether the component is still mounted:

When I first read this, I thought wow, that is very useful. Unfortunately, it does not work as I understood:

• The way I understood it, and thought would be useful, isMounted() returns false if the component has unmounted.
• The way it's implemented, the function will return false is the effect has been cleaned up due to a dependency change.

These are quite different, and I think the first variant is the actually useful one, specifically for:

• Avoiding updating state after unmount.
• Avoiding touching HTML node references that have been removed.

Given how isMounted() currently works, I'm not sure why it's useful.

Would it be possible to fix this? It would be a pretty simple PR I could submit, I think. Only thing would be a major version bump due to breaking change.

Thank you.

Hi, this is a really nice work.

What do you think about adding a "linter" section in documentation ?

Based on this page : https://www.npmjs.com/package/eslint-plugin-react-hooks i think a

    'react-hooks/exhaustive-deps': ['warn', {
      additionalHooks: '(useAsyncEffect)',
    }],

in eslintrc would improve linting.

The library does not work with older browsers (eg. IE 11). It prints in the console SyntaxError: Expected identifier.
Ideally libraries should work without including them in Babel when using them.
Maybe Babel could be used before publishing it to npm?

Thank you for this library it's really straightforward!

I have a question about the possibility of allowing a handler other than React.useEffect. For example, I use useFocusEffect to trigger a data refresh when certain screens come back into focus.

Looking at the source of this library the first (not necessarily best?) solution that comes to mind is adding another export such as useCustomAsyncEffect with the custom effect handler as the first argument. I wanted to see if you have any suggestions or cautions before diving in, in case I'm able to submit a PR.

Hi, it's me again :)

I just wanted to stop by to say I've been trying a new API, and I think use-async-effect could consider supporting the AbortController API. It seemed an opaque API for me before, but after experimenting a bit it seems like a very similar example to the useRef() API:

var controller = new AbortController();
var signal = controller.signal;
console.log(signal);
// { aborted: false, onabort: null }

document.querySelector('body').addEventListener('click', e => {
  console.log(signal);
  // { aborted: false, onabort: null }
  controller.abort();
  console.log(signal);
  // { aborted: true, onabort: null }
});

I am not sure how (if any) to use it, but I can see maybe providing it as a second parameter:

useAsyncEffect(async (isMounted, signal) => {
  try { 
    const res = await fetch(`/users/${id}`, { signal });
    const data = await res.json();
    if (!isMounted()) return;
    setUser(data);
  } catch (error) {
    console.error(error);
  }
}, []);

On the other hand, I think these are bad:

• Mixing try/catch with normal if checks for handling the lifecycle. It seems to be the way fetch() works, but it's very ugly.
• Signal has an onabort callback, which is very similar to the second callback in use-async-effect. Again, two different ways of doing almost the same.
• It can probably be done in user-land in a not-difficult way (going to attempt this soon). The main advantage of doing it this way is the availability of passing it straight into fetch(), which seems the most common case for use-async-effect.

I've rewritten it to be able to return promise of destroyer

import { EffectCallback, useEffect } from 'react';

type EffectResult = ReturnType<EffectCallback>;
type AsyncEffectCallback = () => Promise<EffectResult>;

export function useAsyncEffect(
  effect: AsyncEffectCallback,
  inputs?: ReadonlyArray<any>,
) {
  useEffect(() => {
    const cancelerGetting = effect();

    return () => {
      cancelerGetting &&
        cancelerGetting.then((canceler) => {
          canceler && canceler();
        });
    };
  }, inputs);
}

The newly added isMount function is a breaking change, but has been marked as a feature release. The following code will fail since v2.2.0:

const initialData = { ... }

const fetchSomething = (data = initialData) {
  // Data is now function, but was undefined before
  // The initialData won't be used anymore
}

const onClick = async () => {
  await fetchSomething({ custom: true })
}

useAsyncEffect(fetchSomething, [])

It's now too late, but I just wanted to let you know about this case so you can avoid it in future releases :)

please update the peer dependencies to support React@18

My only VS Code import suggestion for this is:

import useAsyncEffect from 'use-async-effect/types';

@Surely this is not the correct import statement, right?