lambda function passed as mapActions on useRj cause infinite loop
Closed this issue · 0 comments
Cause of useMemo
here:
https://github.com/inmagik/react-rocketjump/blob/master/src/useRj.js#L39
If you pass an anonymous function to useRj
as mapActions
this caused the run
function to have a different instance in every render so if you use it as deps in useEffect
, you got an infinite loop.
Take this example at the first look sounds ok but this ends in a infinite loop:
const MyComponent = () => {
const { _, { run } } useRj(MyRjState, undefined, a => a)
useEffect(() => {
run()
}, [run])
}
You can resolve it by moving the function outside or using useCallback
, these examples have the same effect and works as excepted:
const mapActions = a => a
const MyComponent = () => {
const { state, { run } } useRj(MyRjState, undefined, mapActions)
useEffect(() => {
run()
}, [run])
}
const MyComponent = () => {
const { state, { run } } useRj(MyRjState, undefined, useCallback(a => a, []))
useEffect(() => {
run()
}, [run])
}
Ok, so what to do?
When useRj
was written the mapActions
function was implemented in symmetry to mapActionToProps
in the connectRj
hoc, but while in connectRj
you need this to avoid props collision in useRj
, at the end, this is useless and can lead to sneaky bugs hard to debug.
While selectState
in useRj
was good to use provided selectors from rj
the mapActions
, a the end, is used to rename actions
(i can't see other useful and smarts use cases).
Moreover this can lead to bad code and bad approach to rocketjump
.
Take this piece of code:
const MyComponent = ({ id }) => {
const mapActions = useCallback(({ run }) => ({
run(id),
}), [id])
const { state, { run } } useRj(MyRjState, undefined, mapActions)
useEffect(() => {
run()
}, [run])
}
This is formally correct and in fact do what you do think do, but is very cumbersome, and if you forget the useCallback
you return to the problem explained above: an infinite loop.
The correct refactoring is:
const MyComponent = ({ id }) => {
const { state, { run } } useRj(MyRjState)
useEffect(() => {
run(id)
}, [run, id])
}
Ok, after all these thoughts, my proposal is to TOTALLY REMOVE mapActions
in useRj
API.
If you want to rename actions simply use object deconstructing:
const MyComponent = ({ id }) => {
const { state, { run: loadMyAwesomeState } } = useRj(MyRjState)
useEffect(() => {
loadMyAwesomeState(id)
}, [loadMyAwesomeState, id])
}
... Or you can even use the rj
API!
const MyRjState = rj({
effect: myApi,
actions: ({ run }) => ({
loadMyAwesomeState: run,
})
})
const MyComponent = ({ id }) => {
const { state, { loadMyAwesomeState } } = useRj(MyRjState)
useEffect(() => {
loadMyAwesomeState(id)
}, [loadMyAwesomeState, id])
}
Let me know what do you think, and if you have a valid use case for mapActions
.
If you agree with me i will remove mapActions
in the 1.1.0
release of react-rocketjump
.
Thanks for the time, again... 😄