stimulus_reflex icon indicating copy to clipboard operation
stimulus_reflex copied to clipboard

Provide configurable Reflex object cleanup hook

Open leastbad opened this issue 3 years ago • 2 comments

Just moving this out of #592, and stashing it as a TODO issue.

As @hopsoft said in this comment:

I wonder if we should set a max limit for how many reflexes will be stored to prevent memory bloat given that the Reflex class holds references to DOM elements and Stimulus controllers. I worry that this may prevent some needed garbage collection over time. We could cleanup the references as reflexes get pushed out of the store.

Maybe we make it configurable and default to 1000? We could also leave it unlimited in debug mode?

To which I replied:

There's two high-level approaches possible:

  1. cleanup executed as part of the final stage of each Reflex
  2. launch a setInterval that runs globally, purges every n seconds (configurable via initialize option)

Both approaches would execute a callback (with a default provided), with overrides passed as an option to StimulusReflex.initialize().

Which path do you prefer?

As for the default callback, there's lots of ways to slice it:

  • do nothing
  • nuke reflex(es) right away
  • nuke reflex(es) setTimeout(n) seconds after they are finalized
  • nuke right away if not the most recent Reflex
  • nuke by FIFO queue as you suggest, with n size configurable

The cool thing is that we really could make the default () => {} and then give examples of all of the others in the docs, with an explanation about why they might want to clean up. In some ways, I like defaulting to a null function because of the principle of least surprise.

leastbad avatar Sep 10 '22 07:09 leastbad

@marcoroth since we’re thinking about removing/reworking #592, should we consider closing this?

julianrubisch avatar Feb 15 '23 07:02 julianrubisch

I think we can keep #592 as-is!

We probably should just implement what's being described here.

marcoroth avatar Feb 15 '23 12:02 marcoroth