-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
useEffect can very quickly consume free quotas or cost money when used with 3rd party services #15188
Comments
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 #15184 and #15180 but these are mostly for sync loops. Catching async ones is way harder. |
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>
);
} |
I'm not familiar with Firebase — could you explain more about why there are two effects, and what was the intended logic? Thank you. |
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. |
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. |
I see what you mean. For me the solution is to use a custoom hook as 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 |
I love react, but I'm melting Firebase quota all the time in this pit. |
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. |
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.
The text was updated successfully, but these errors were encountered: