cassiozen / useStateMachine

The <1 kb state machine hook for React
MIT License
2.38k stars 47 forks source link

Prevent memory leaks #74

Closed devanshj closed 2 years ago

devanshj commented 3 years ago

Consider this example -

const Something = () => {
  let machine = useStateMachine({
    initial: "subscribed",
    states: {
      subscribed: {
        effect: () => {
          let unsubscribe = spawnSomeService();
          return () => unsubscribe()
        }
      }
    }
  })
}

Does anything look fishy here? No right? Except it has a memory leak. The effect cleanup never runs, even after Something is unmounted. So if it's remounted 10 times through out the lifetime of the page, then 10 services would be running instead of 1.

To prevent the memory leak one has to do something like this -

const Something = () => {
  let machine = useStateMachine({
    initial: "subscribed",
    states: {
      subscribed: {
        effect: () => {
          let unsubscribe = spawnSomeService();
          return () => unsubscribe()
        },
        on: { UNSUBSCRIBE: "unsubscribed" }
      },
      unsubscribed: {}
    }
  })

  useEffect(() => {
    return () => machine.send("UNSUBSCRIBE")
  }, [])
}

This is not intuitive at all. Not to mention the fact that the effects don't cleanup on unmount is very misleading. So people probably will be having memory leaks in their apps unknowingly.

You see other libraries don't call it "effect" they call them "entry" and "exit". Latter does not have to guarantee that each node that has been entered will always be exited but former has to guarantee that every effect's cleanup will run because the terminology and effect-looking api it chose (which is a good choice as it makes things like this more apparent and cleanups easy to write because the unsubscribers will be in the closure)

So to solve this I propose we have a $$stop event (and rename $$initial to $$start) which would be sent when the component gets unmounted and will basically exit all nodes upto the root (so it'll only show up in cleanup's event parameter and not effect's).

And having a stop event makes sense if you think of spawning the machine one big effect, if it has a start event and does some effects, it should also have a stop event that disposes stuff spawned by the effects.

Trivia: I was writing the hierarchical implementation where I realized root is the only node which would never exit no matter what, and thought maybe that's why we don't have effect on root as it won't cleanup. But then I realized that's true for all nodes that are active before unmounting. It's funny I didn't plan to run the root effect, the recursive abstraction demanded me to do it. That's what I like about fp that it demands some consistency, some symmetry, some pattern and if there's something fishy and you don't have these things, then you'd catch it right off the bat as you'd have to go out of your way to do something asymmetric.

cassiozen commented 3 years ago

This is a very good point. To me, it was immediately obvious that the cleanup on the initial state would not run, but I do have inner knowledge of the library and I agree it's not intuitive at all.

So to solve this I propose we have a $$stop event (and rename $$initial to $$start) which would be sent when the component gets unmounted and will basically exit all nodes upto the root (so it'll only show up in cleanup's event parameter and not effect's).

That is a very clever solution. And it makes me feel so much better about having the $$initial event in the first place.

And having a stop event makes sense if you think of spawning the machine one big effect

Yes, absolutely. And it's also a better match for what the user would expect. And it even pairs better with something that will be inside a React component.

Trivia:

Hahaha, love it. Again, I'm learning a lot from your contributions.

devanshj commented 3 years ago

but I do have inner knowledge of the library and I agree it's not intuitive at all.

Exactly

And it makes me feel so much better about having the $$initial event in the first place.

Haha I'm glad

it even pairs better with something that will be inside a React component.

Yep, exactly

I'm learning a lot from your contributions.

Haha thanks a lot, humbled! :)

devanshj commented 2 years ago

Why would you close this haha? Shouldn't we keep this open till we get $$stop? (Feel free to rename the issue to be less alarming like "Clean up all effects on unmount" or something like that)