autorun destruct handling

[martinheidegger]

I have an idea that could solve a code cleanness issue (and by extend might result in less bugs in userland).

What if autorun (or a variant of it) would process eventually returned functions in the destruct process and before subsequent runs:

const obj = observable({ foo: 'bar' })
const destruct = autorun(() => {
  console.log(`render ${obj.foo}`)
  return () => {
    console.log(`cleanup ${obj.foo}`)
  }
})
// render bar
obj.foo = 'baz'
// cleanup bar
// render baz
destruct()
// cleanup baz

As illustrated in the code, the first time autorun is called it would just process the code. The return function would be stored and executed (if possible with the old model state, thought that might be difficult).

Right now I need to keep track manually if the method has run before and wrap the destruct mechanism with duplicate code. This makes working with autorun in my world a bit hard. This should be useful in a variety of use-cases as such I think it might be interesting to other users as well.

I am not sure if this can or should be implemented, I am not familiar with the repository but I would be willing to work on a PR with a little guidance.

[danielkcz]

I don't exactly follow what is your reasoning here. The autorun is basically one-off that runs whenever the observable would change. If you would create some resource inside of it that needs to be cleaned up during that run, where do you keep it?

Also, it would be prone to memory leaks. You never know how many times would reaction execute before you dispose it. Some of those created resources would be left hanging in there.

Of course, it would be possible to keep a reference to all cleanup functions which means keeping alive whole closure. But that means you would be creating the same resource on every reaction execution. That's pretty wasteful.

I don't think this is a job for MobX, I might be wrong though. Not sure if you are using React, but something similar should be possible in any sort of library/framework.

React.useEffect(() => {
  // setup other resources
  const dispose = autorun(() => {
    // observe
  })
  return () => {
    dispose()
    // cleanup other resources
  }
})

That way you can be sure there are no memory leaks or weird behavior.

[mweststrate]

I think it is pretty easy to create your own abstraction for this with the existing primitives, and there is no reason to add it to the core is it doesn't look very common. So you could establish your own autoRunOnceCleanup around the above pattern as utility. Note that for one-off effects when is usually used. I'm curious what your actual use case is, as rendering is typically a repeating process?

[martinheidegger]

My actual use-case is to reinitialize the connection to the server when the specified (observable) config changes, a little bit like:

useEffect(() =>
  autorun(() =>
    listenToChannel(config.channelId, handler) // listenToChannel returns a destructor as well
  )
)

in this case the channel listener would be added/removed every time the config of the channel has changes and every time the component is added/removed from the UI. (in other words: no lost listeners).

So you could establish your own autoRunOnceCleanup around the above pattern as utility.

I actually did that already, though I called it safeAutorun. It is in use in my main mobx-keystone app that uses a configuration here to create a new subscription when the server address changes in the config.

My thinking was that this util/pattern might be useful in other cases as well: think UI listening such as BackHandler.addEventlistener() which is why I thought it might have its place in mobx in general.

[danielkcz]

Yea, I don't think we want to expand API layer for rare cases that can be tackled easily in user-land. I can also imagine it could lead to bad practices and abuse of autorun for something it's not meant for.

Your solution looks ok, so let's close it here.

[mweststrate]

I think if you set up the deps of useEffect here correctly, there is no need for auto autorun here at all?

Op zo 19 jan. 2020 11:30 schreef Daniel K. notifications@github.com:

Closed #2258 https://github.com/mobxjs/mobx/issues/2258.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2258?email_source=notifications&email_token=AAN4NBASLDS4LMWZ5KFU243Q6Q2VTA5CNFSM4KIWTWV2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOWB5K6SA#event-2960830280, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBH6ELD22DNIFGV27H3Q6Q2VTANCNFSM4KIWTWVQ .

[martinheidegger]

I am not sure if I was able to make my case properly or not but I thank you anyways for the consideration.

[danielkcz]

Yea, the "problem" is that it's your single case and over several years I am not aware of anybody asking for something similar. We won't be bloating API because it feels convenient for a single case really. I hope you can understand. If you can gather more people with similar cases then it's something for consideration.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.