inmagik/react-rocketjump

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... 😄