Closed joshunger closed 4 years ago
A much better fix is usually to either move a function either outside of the component (if it doesn't use props or state), or move it inside the effect (if it does). useCallback
is the escape hatch for when you can't or don't want to do either. So I don't think we'd want to autofix it to useCallback
every time.
We could offer an option that disables the autofix when we detect function deps.
That would be a better fix. I think a lot of people end up on your original code example https://github.com/facebook/react/issues/14326#issuecomment-441680293.
You could leave autofix if there was a better solution for detecting infinite async loops.
Thanks, I updated that example.
@gaearon I'm sorry for the completely unrelated question in the topic (feel free to delete it)
But is the extraneous-deps
rule is stable now to use, and all problems and false positives have been sorted?
We've been using it at FB for a few weeks now and are pretty happy with it.
I have a problem when --fix
is enabled on this line. It changes the deps to:
const ref = useRef<HTMLElement>(null);
const [shortcuts, setShortcuts] = useState<ShortcutAction[]>();
useLayoutEffect(() => {
const trapper = scoped && ref.current ? new mousetrap(ref.current) : mousetrap;
const map = mapKey ? (shortcutMap[mapKey] as Shortcut) : (shortcutMap as Shortcut);
const shortcutActions = buildShortcuts(map);
shortcutActions.forEach((shortcut) => {
trapper.bind(shortcut.keys, (e: ExtendedKeyboardEvent) => {
e.preventDefault();
e.stopPropagation();
handler(shortcut.action, e);
});
shortcut.trapper = trapper;
});
setShortcuts(shortcutActions);
return () => {
if (!shortcuts) {
return;
}
shortcuts.forEach((shortcut) => {
if (shortcut.trapper) {
shortcut.trapper.unbind(shortcut.keys);
}
});
};
}, [handler, mapKey, scoped, shortcutMap, shortcuts]);
Adding the shortcuts
dep will cause an infinite loop as setShortcuts
is called which will cause a rerender.
I have this shortterm fix:
const previousHandler = usePrevious(handler);
useLayoutEffect(() => {
if (shortcuts && handler === previousHandler) {
return;
}
I very well could be approaching this wrong :)
Maybe I'm missing something, but how would I prevent infinite loops in this example?
By adding onChange()
as a dep, changing the "Primary Input" goes into an infinite loop.
EDIT:
Solved it with useCallback()
. Here's the CodePen.
Hello, the eslint rule is creating an infinite loop in my case too:
I'm guessing there is a better way to do what i do without the infinite loop and disabling eslint.
TLDR:
on my React app, i can receive the code
for oauth2.
when i do, i make a fetch to my backend to get an access_token
, then i register the token in localStorage.
If i have a token or the token change, i make a call to my backend to get a list of artists.
In both cases, i use in the useEffect
: setArtists
or setToken
and the eslint rules add them to the array, which creates an infinite loop.
PS: the solution for @goodoldneon is not working in my case, when i copy his code into my editor, the autoformat is adding the setX
into the array of useCallback
and will create the same issue.
@wcastand, is your editor saying you need the useState()
set functions as deps in useCallback()
?
Yeah, i'm using vscode if that helps. The auto format/fixing is adding the setters and vars in the array and that creates an infinite loop.
I added //eslint-disable-next-line
and now it works but that feels like a hack lol
@wcastand The correct fix in your case is to wrap customSetState
that’s returned from your custom Hook into useCallback
. Then it can safely be specified as a dependency (like it should be).
Please read the documentation which explains all of these cases one by one:
Thanks for the help, sorry for the delayed answer, i'll try this out when i can but the project was based on spotify api and i am hitting limit rate for various reason and limitation of their API.
Anyway thanks for the example, this issues won't be issues anymore after Suspense and cache
are introduced for React right?
@gaearon I made a reproduction of a custom hook to work with array: https://codesandbox.io/s/hopeful-meadow-629zh
The set function is re-exported from the dipatcher of useState(), however eslint throw error as I'm calling it from the object. Wonder what is your thought as to whether this is a valid case?
My thinking is that I could eitehr useReducer, or export my hook as an array, or memo the object I'm exporting from my hook :-? ...
Yeah you can memo that object.
It would probably make sense to me if you didn’t keep using the object directly. But destructure individual functions from it instead. Then they can be more stable (for example a setter would never need to change). You could useCallback for those individual functions.
Regarding the autofix, I hope https://github.com/eslint/rfcs/pull/30 will let us solve the problem without compromising on IDE suggestions.
Let's consolidate and track this in https://github.com/facebook/react/issues/16313 instead.
This is resolved now. https://github.com/facebook/react/issues/16313#issuecomment-587149109
I've read "My function value is constant" from https://github.com/facebook/react/issues/14920#issuecomment-471070149.
If you autofix the
useEffect
, would it also be possible to autofix any functions added by wrapping them in auseCallback
at the same time?This would greatly improve the user experience.