Rename useObservableEffect to useDisposable

[danielkcz]

@xaviergonz I just realized that useObservableEffect might not be needed. The only benefit I see is that you get the disposer to the outer scope, but I am not sure about a single use case scenario where that could be useful (yet).

This will work exactly the same if I am not mistaken since reaction/autorun/when are all returning a function that will be called on unmount anyway. Having a memoized ref to that disposer is such a tiny bonus.

   useEffect(() =>
       reaction(
           () => [props.firstName, props.lastName],
           () => {
               // send this to some server
           }
       )
   , [])

It's not like it's completely useless, but feels like it might confuse users that there is some other benefit to it. Correct me, if I am wrong, please.

[xaviergonz]

doesn't use Effect run for the first time after the first render, layout and paint is finished?

[xaviergonz]

The closest would be useLayoutEffect which is kind of like componentdidmount, but I think people would expect it to run ASAP if they specify things such as fireImmediately.

Could be written some other way? Sure, but I think it is fair to give the users a default implementation so they don't fall in those kind of traps

[danielkcz]

@xaviergonz I gave some thought to this and you are right, that underlying useMemo helps to have an observable effect setup as soon as possible, so that's a good thing.

However, I think that it's also confusing naming. I know, it was my suggestion instead of useMobXEffect, but I haven't seen it before.

In the essence that hook not really specific to MobX. I think a name like useDisposable could be better and revert use of type IReactionDisposer which I have suggested 🙈. What I mean is that it can be a rather universal hook to handle anything that returns a dispose function.

[danielkcz]

As the example goes, this is also useful in MobX world, but due to TypeScript it's not compatible with Lambda type returned by observe.

const state = useObservable(new Map())
useObservableEffect(() => state.observe((change) => ... ))

[danielkcz]

Well, since I essentially needed that last example I just went and did rename it and released in 0.3.3. The useObservableEffect is still there, just using useDisposable underneath.

[xaviergonz]

Why not remove useObservableEffect? It is still v0

[danielkcz]

I know, but I do use in various places and don't want to handle right now 😎

[mweststrate]

Random curiosity, did anybody encounter cases where useEffect wouldn't have sufficed?

[danielkcz]

I am using useDisposable fairly often when I need to imperatively execute some side effect based on a reaction. I even have a useReaction abstraction to simplify these things. To replace that with useEffect I would have to specify dependencies manually and make sure I have observer HOC (won't work with other two variants). Not even mentioning it feels like overkill to re-render the whole component just to execute some side-effect :)

[mweststrate]

curious, could you share an example? useEffect(() => autorun(() => { })) wouldn't trigger new renders either, correct? (useReaction seems need as you avoid the double function wrapping)

…

On Fri, Mar 22, 2019 at 2:25 PM Daniel K. ***@***.***> wrote: I am using useDisposable fairly often when I need to imperatively to execute some side effect based on a reaction. I even have a useReaction abstraction to simplify these things. If I would like to replace that with useEffect I would have to specify dependencies manually and make sure I have observer HOC (won't work with other two variants). Not even mention it feels like overkill to re-render the whole component just to execute some side-effect :) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#28 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhOa09xJND1m7f4MF4r-N3VimLlGkks5vZNnggaJpZM4Yq71L> .

[danielkcz]

Ah, I see what you mean now. That's curious :) We have fairly non-trivial logic in the useDisposable implemented by @xaviergonz in #46. There is a tiny benefit that it returns a disposer, but honestly, I haven't used that once, so it's probably useless. I guess we got a little overboard with that one without thinking in basics.

[danielkcz]

Out of curiosity, I tried to strip down the implementation of useDisposable to the useEffect call and only a single test checking for that returned disposer fails. I guess we can easily deprecate this helper with a hint to apply useEffect instead. Thanks, @mweststrate, that was interesting eye opener :)

[mweststrate]

I had a slide on it in my workshop, and was preparing for what to answer if someone asked "why no useEffect?"😇 Op vr 22 mrt. 2019 15:45 schreef Daniel K. <[email protected]>:

…

Out of curiosity, I tried to strip down the implementation of useDisposable to the useEffect call and only a single test checking for that returned disposer fails. I guess we can easily deprecate this helper with a hint to apply useEffect instead. Thanks, @mweststrate <https://github.com/mweststrate>, that was interesting eye opener :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhHqeee2GnptgArkepnCFbde_L7Ieks5vZOyHgaJpZM4Yq71L> .