search

CSFrequency / react-firebase-hooks

React Hooks for Firebase.
Apache License 2.0
3.58k stars 305 forks source link

Firestore use*Once methods seem to be susceptible to race conditions and "state update on an unmounted component". #194

Closed mrtnkb closed 2 years ago

mrtnkb commented 2 years ago

This looks very similar to https://github.com/CSFrequency/react-firebase-hooks/issues/35, but that was marked fixed in 2019.

For slow queries and fast UI changes it is possible for either the wrong result to be displayed or "state update on an unmounted component" warnings to be shown.

This seems to be caused by the async state update in the *Once method useEffects. setValue is called asynchronously and if the get responses are returned out of order, the value state will be set to the wrong result. It can also trigger an invalid call after a component is unmounted.

get(ref.current).then(setValue).catch(setError);

A possible solution would be to have these listen==false branches return a cleanup which would cause the get response to be ignored. A work around is switching to using the non-*Once methods.

https://github.com/CSFrequency/react-firebase-hooks/blob/11aa6dc474743ff1db44dbb8342c73f463fa4566/firestore/useDocument.ts#L95 https://github.com/CSFrequency/react-firebase-hooks/blob/11aa6dc474743ff1db44dbb8342c73f463fa4566/firestore/useCollection.ts#L95

chrisbianca commented 2 years ago

@mrtnkb how do you expect the get responses to be returned out of order? get returns the latest value of the document or collection once and only once.

The invalid call after a component is unmounted is a fair challenge, although is a no-op in React so won't actually harm your application.

mrtnkb commented 2 years ago

Isn't it because under the hood they're network requests? There aren't any guarantees on how long each will take to get serviced and of two queries placed in quick succession (e.g. in response to rapid UI state changes) the second query then clause could get called first - leaving setValue to be called with the now invalid response from the first query.

You can reproduce this problem if you set up a component that triggers different collection queries depending on a some UI state. If one state results in a simple query with only a few document responses and another state results in a complex query with a lot of document responses you can cause the hook to return the wrong value by toggling the state quickly.

One solution to both problems would be to track if the response corresponds to the current request:

    } else {
      let valid = true;
      const get = getDocsFnFromGetOptions(
        options ? options.getOptions : undefined
      );
      get(ref.current).then((value) => {
           if (valid) { setValue(value); }
        }).catch(setError);
      return () => { valid = false; };
    }
chrisbianca commented 2 years ago

@mrtnkb Ok, that makes sense. I was just struggling to see how you'd end up in a race condition, but you're right this could happen when you're changing the query within a component as described. I can take a look into this for a future release.

mrtnkb commented 2 years ago

That would be awesome! Or if you accept contributions/have a guide for new contributors, then I'd be happy to send a PR for you to look at.

chrisbianca commented 2 years ago

@mrtnkb more than happy to accept contributions and review them. There aren't any guidelines as such, just make sure you have prettier running if possible.

chrisbianca commented 2 years ago

This has been fixed and released as part of https://github.com/CSFrequency/react-firebase-hooks/releases/tag/v4.0.2

Thanks @mrtnkb for highlighting and resolving the issue