facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.31k stars 46.96k forks source link

useEffect can very quickly consume free quotas or cost money when used with 3rd party services #15188

Closed PutziSan closed 3 years ago

PutziSan commented 5 years ago

Do you want to request a feature or report a bug? Feature / Documentation-Request

What is the current behavior? When I developed my app last week with useEffect and firebase firestore, it happened to me that my effect used up the 20k-write limit within about 20 seconds. Of course this was a bug introduced by myself, but if I had been in a pay-as-you-go plan it could have cost me some money. I now use a custom hook as useEffect, which counts in development whether a hook is executed too often in 500ms and if so, it throws an error.

What is the expected behavior? I'm not sure how you could solve this on your side. Of course you could do the check in development-mode, but that would probably trigger existing projects too much. However, a small hint in the documentation would be good that you should take care during development that useEffect can quickly lead to this behavior and that you should be careful when using other 3rd-party services that have a quota or have to be paid.

I just wanted to share my experiences while developing a "real" app. If you can't or won't do anything here, you are welcome to close the issue.

gaearon commented 5 years ago

Yeah, this has been bugging me too. To be clear it’s not entirely a new problem. The same could happen if you unconditionally fetched in componentDidUpdate and then added an animation to a parent component. But I agree it comes up more with useEffect.

Can you share the exact code snippet you used? It’s kinda tricky to catch early. I am adding mitigations for some cases in https://github.com/facebook/react/pull/15184 and https://github.com/facebook/react/pull/15180 but these are mostly for sync loops. Catching async ones is way harder.

PutziSan commented 5 years ago

Can you share the exact code snippet you used?

Do you mean the snippet which caused the bug? This is the condensed variant:

function Comp(props) {
  const [data, setData] = useState();

  useEffect(async () => {
    if (!data) {
      return;
    }

    const newData = {
      timestamp: Date.now(),
      content: data
    };

    await firebase
      .firestore()
      .collection("test")
      .doc(props.id)
      .update(newData);
  }, [data]);

  useEffect(() => {
    const unsubscribe = firebase
      .firestore()
      .collection("test")
      .doc(props.id)
      // the bug:
      // onSnapshot changes data, whereupon the other effect is activated,
      // which again triggered onSnapshot, .....
      // and onSnapshot is also called synchronously:
      // "Local writes in your app will invoke snapshot listeners immediately."
      // https://firebase.google.com/docs/firestore/query-data/listen
      .onSnapshot(snapshot => {
        setData(snapshot.data().content);
      });

    return unsubscribe;
  }, [props.id]);

  return (
    <div>
      <input onChange={e => setData({ msg: e.target.value })} />
    </div>
  );
}
gaearon commented 5 years ago

I'm not familiar with Firebase — could you explain more about why there are two effects, and what was the intended logic? Thank you.

PutziSan commented 5 years ago

The principle is something like google docs: We have one document and 2 users, both users can write in the document at the same time and see each other's changes "in real time".

And the 2 effects are correspondingly there for the 2 actions: write into the document at own changes and read from the document when the other user writes into the document.

The problem was that the first time I implemented it, I immediately got my own changes mirrored back, which then leads to an infinity loop.

gaearon commented 5 years ago

Do you think it could be something that Firebase bindings could handle? There are many other cases where you could accidentally call something in a loop. It seems like Firebase library itself could warn or error because it’s obvious you don’t want to fire off hundreds of requests in a second. In other words it would make sense to me if the error happened close to where we actually know the behavior isn’t desirable — which is right next to the network layer.

PutziSan commented 5 years ago

I see what you mean. For me the solution is to use a custoom hook as useEffect. This is faster and easier to implement for me and then also applies to all other cases. I have to opt-in to allow an effect to really be called more often than it should be "normal". I have defined "normal" as an estimate for myself: 5 calls within 500 ms. This seems to work quite well so far, it fails if it wasn't correct and otherwise it goes through.

Here is the code which I use to "observe" my effects (or you can take a look at this CodeSandbox-demo):

function useObservedEffect(
  fn,
  deps,
  { maxCalls = 5, maxCallIntervalMs = 500 } = {}
) {
  /* eslint-disable react-hooks/exhaustive-deps */
  // effects inside if is okay here, since the result of
  // `process.env.NODE_ENV === "development"` is static per build
  if (process.env.NODE_ENV === "production") {
    useEffect(fn, deps);
  } else {
    // in development-mode, check if the effect will be called too often
    const callCountRef = useRef(0);
    const [error, setError] = useState();

    if (error) {
      throw error;
    }

    // Reset the counter every X milliseconds so we count the calls only per interval
    useInterval(
      () => {
        callCountRef.current = 0;
      },
      // if maxCallIntervalMs === 0, the verification should not be performed.
      // In these cases, set the interval to the maximum possible value
      // so that it is not executed senselessly often.
      maxCallIntervalMs > 0 ? maxCallIntervalMs : Number.MAX_VALUE
    );

    useEffect(() => {
      // Increase the counter corresponding to the calls in the last interval.
      callCountRef.current++;

      // if maxCallIntervalMs === 0, the verification should not be performed.
      if (maxCallIntervalMs > 0) {
        if (callCountRef.current > maxCalls) {
          // if the effect is called too often, set an error and do not execute the provided effect-function
          // the error will throw in next render
          setError(new Error(`... (error-message)`));
          return;
        }
      }

      fn();
    }, deps);
  }
}

I cannot see any disadvantages in this approach for me, but I could imagine that it could not fit so well into the react-package directly, because effect functions suddenly react differently in development than in production. Some users might not find this good

elzup commented 3 years ago

I love react, but I'm melting Firebase quota all the time in this pit.

PutziSan commented 3 years ago

I would recommend to use https://github.com/kentcdodds/stop-runaway-react-effects nowadays. It is basically my proposed solution from above, but overwrites the useEffect hook directly during development.