mobxjs / mobx-react

React bindings for MobX
https://mobx.js.org/react-integration.html
MIT License
4.85k stars 350 forks source link

Clearing up behaviour with Autorun in Effect using different Observers. #848

Closed seivan closed 4 years ago

seivan commented 4 years ago

Potential bug, but not 100% certain.

I was hoping to clear up some issues not mentioned in the mobx-react docs based on implementation and outcomes.

There seems to be different behaviour between <Observer> and useObserver() when using effects and autoruns. Even different behaviour within useObserver() as well. So let me draw up the different ways and go through what happens.

Omitting the returned render body for brevity, but defining what approach is used.

The Observable will be passed as props to the components defined below.

const person = observable({
  name: 'John'
})



useObserver

1 autorun in useEffect with an empty dependency array as documented.

  React.useEffect(() => {
    return autorun(() => {
      const name = person.name;
      const message = `Autorun Change ${name}`;
      console.log(message);
    })
  }, []);

It works fine, but there is an annoying warning about the used property person.name is missing from the array. What's the approach here, do you silence the specific warning?



2 autorun in useEffect without an array.

  React.useEffect(() => {
    return autorun(() => {
      const name = person.name;
      const message = `Autorun Change ${name}`;
      console.log(message);
    })
  }); //No empty array. 

The autorun will get triggered N+1 times. Meaning , if the property is changed 4 times, the 4th time will trigger 3 times +1

person.name = "firstChange" //triggered 1 time
person.name = "secondChange" // triggered 2 times
person.name = "thirdChange" // triggered 3 times

This is a dangerous behavior and easy to miss. This doesn' happen when returning <Observer> components, more about that later.



3 useEffect without autorun and no dependency list defined.

  React.useEffect(() => {
      const name = person.name;
      const message = `Autorun Change ${name}`;
      console.log(message);
    })
  });

Oddly enough this works perfectly. Only gets triggered when person.name actually changes and would be my preferred choice, but I need to know if there are any issues going with this approach. This has no warning, isn't triggered multiple times, and doesn't need to define the dependency list, this would be my preferred choice instead of nesting two closures making it verbose and cluttered.

Could it be the reason this works is because useEffect are only run when a render is about to happen, and a render will not happen unless person.name changes. I assume if I used a different property in the effect that wasn't used in the render body we'd have a different behaviour?



<Observer> component returned.

autorun inside a useEffectwithout the dependency list, same as #2 but actually working as intended.

const P2 = ({ person }) => {
  React.useEffect(() => {
    return autorun(() => {
      const p = person.name;
      const x = `Autorun Change ${p}`;
      console.log(x);
    })
  });

 return <Observer>{() => <h1>{person.name}</h1>}</Observer>
}

As mentioned this hooks behaves differently when used with a useObserver hook as a returned body.

I hope to get some clarification here as the docs are very ambiguous between what works and doesn't and differences between approaches, especially between Observable approaches. I haven't tested these differences with HOC, what to clear this up first.

I wonder if it's related to this issue https://github.com/mobxjs/mobx-react/issues/849 which is why they behave differently and seemingly unpredictable.

You can try this in the sandbox but I also changed mobx-react and mobx to the latest versions.

mweststrate commented 4 years ago

See https://mobx-react.js.org/observe-how.

Use observer() whenever you can, <Observer> for for example callback components where you don't have control over the rendering component. useObserver is mostly an internal api, and personally I don't know any use case where it comes in handy

On Fri, Mar 27, 2020 at 9:44 AM Seivan notifications@github.com wrote:

I was hoping to clear up some issues not mentioned in the mobx-react docs based on implementation and outcomes.

There seems to be different behaviour between and useObserver() when using effects and autoruns. Even different behaviour within useObserver() as well. So let me draw up the different ways and go through what happens.

Omitting the returned render body for brevity, but defining what approach is used. The Observable will be passed as props to the components defined below.

const person = observable({ name: 'John' })

useObserver 1 autorun in useEffect with an empty dependency array as documented.

React.useEffect(() => { return autorun(() => { const name = person.name; const message = Autorun Change ${name}; console.log(message); }) }, []);

It works fine, but there is an annoying warning about the used property person.name is missing from the array. What's the approach here, do you silence the specific warning?

2 autorun in useEffect without an array.

React.useEffect(() => { return autorun(() => { const name = person.name; const message = Autorun Change ${name}; console.log(message); }) }); //No empty array.

The autorun will get triggered N+1 times. Meaning , if the property is changed 4 times, the 4th time will trigger 3 times +1

person.name = "firstChange" //triggered 1 timeperson.name = "secondChange" // triggered 2 timesperson.name = "thirdChange" // triggered 3 times

This is a dangerous behavior and easy to miss. This doesn' happen when returning components, more about that later.

3 useEffect without autorun and no dependency list defined.

React.useEffect(() => { const name = person.name; const message = Autorun Change ${name}; console.log(message); }) });

Oddly enough this works perfectly. Only gets triggered when person.name and would be my preferred choice, but I need to know if there are any issues going with this approach. This has no warning, isn't triggered multiple times, and doesn't need to define the dependency list, this would be my preferred choice instead of nesting two closures making it verbose and cluttered.

component returned. autorun inside a useEffectwithout the dependency list, same as #2 but actually working as intended. const P2 = ({ person }) => { React.useEffect(() => { return autorun(() => { const p = person.name; const x = `Autorun Change ${p}`; console.log(x); }) }); return {() =>

{person.name}

}
} As mentioned this hooks behaves differently when used with a useObserver hook as a returned body. I hope to get some clarification here as the docs are very ambiguous between what works and doesn't and differences between approaches, especially between Observable approaches. I haven't tested these differences with HOC, what to clear this up first. You can try this in the sandbox — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub , or unsubscribe .
seivan commented 4 years ago

@mweststrate Sorry, but that doesn't explain much here related to the useEffect & autorun (general sideeffects)

Even the second sentence says

In most cases, their difference doesn't matter that much, but here is an overview of the three different approaches

Obviously they seem to matter with breaking distinctions.

I've gone ahead and tested adding more properties and making distinction between what's part of the side/effect/autorun and what's part of the rendered body and the behaviour gets even more erratic.

I was hoping to get some actual explanation so we can be careful about these things.

mweststrate commented 4 years ago

Make sure you understand both useEffect and autorun themselves first, as your current example doesn't make much sense as it will recreate a new autorun on every render. For autorun, the general MobX observation rules apply: https://mobx.js.org/best/react

observer: wraps a component in 'autorun' to render it whenever an observable change Observer: instantiates a fresh component that itself is an observer, so tracks the function you pass, and re-render whenever observables used in it change. But be aware that that closure might trap values that are not observables useObserver: tracks only the specific function (rather than the entire component body like observer), and re-render the whole component if some observable used by that function changes. Again beware of trapping values in the closure

On Fri, Mar 27, 2020 at 10:16 AM Seivan notifications@github.com wrote:

@mweststrate https://github.com/mweststrate Sorry, but that doesn't explain much here related to the useEffect & autorun (general sideeffects)

Even the second sentence says

In most cases, their difference doesn't matter that much, but here is an overview of the three different approaches

Obviously they seem to matter with breaking distinctions.

I've gone ahead and tested adding more properties and making distinction between what's part of the side/effect/autorun and what's part of the rendered body and the behaviour gets even more erratic.

I was hoping to get some actual explanation so we can be careful about these things.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/848#issuecomment-604920946, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBHZVWKM4O4AUV65UADRJR4GRANCNFSM4LU34G6A .

mweststrate commented 4 years ago

Regarding "In most cases, their difference doesn't matter that much, but here is an overview of the three different approaches"

@FredyC can we update the docs? I think explaining the options as kinda equals just adds confusion. Let's make clear the recommendation is observer and <Observer> for the case that don't fit that. Personally I'm still doubtful we should expose useObserver as all, but it was important for one of your abstractions IIRC?

seivan commented 4 years ago

@mweststrate Thanks for getting back so quickly, appreciate it.

Make sure you understand both useEffect and autorun themselves first

I understand how they work, the reason I brought up the examples was different behaviour. Also what particular example are you referring to, as I gave 3 with useObserver and one with <Observer>


observer: wraps a component in 'autorun' to render it whenever an observable change

Yep, you lose out on the granular control <Observer> gives, but it's clearer to make the whole component tracking the observables.


Observer: instantiates a fresh component that itself is an observer, so tracks the function you pass, and re-render whenever observables used in it change.

Yep, this I understand.


But be aware that that closure might trap values that are not observables

Correct, but if the traps values that are not observables, on a re-render it will reflect changes in those values. E.g person.name is observable, and post.title is not, if name changes, it will re-render the whole <Observer> body (but not the parent wrapping <Observer>!)


useObserver: tracks only the specific function (rather than the entire component body like observer), and re-render the whole component if some observable used by that function changes. Again beware of trapping values in the closure

Yep, this was as mistake on my part as I misunderstood: One good thing about this is that if any hook changes an observable for some reason then the component won't rerender twice unnecessarily. (example pending)

That being said, I can totally see the use case:

<Observer> lets you control updates and tracking. Lets you render old values in the same component using <Observer>, even if tracked values are updated.

<useObserver> lets you control tracking Lets you render old values, as long as anything inside the closure doesn't change observables and trigger a re-render.

as your current example doesn't make much sense as it will recreate a new autorun on every render.

Which particular example? I noticed when using trace() that autoruns were disposed and recreated (as you said) with a few of those.

But I am missing an explanation on what the correct approach is, following the official documentation I get a React warning, and nothing about how to deal with.


For autorun, the general MobX observation rules apply: https://mobx.js.org/best/react

Those are not explained in combination with Reacts rendering and hooks rules.




I guess to break it down:

Using autorun in useEffect following an empty dependency list as documented in combination with useObserver will not work as intended - unless I am mistaken, which is why I see your hesitation to expose it.

So if we remove useObserver from the equation, why is autorun() in an useEffect necessary if you are using observer() as the whole component is wrapped in an autorun or is it just the returned rendered body/jsx?

Even then, would observer() recreate a new autorun inside an useEffect on every render? Then why is the docs recommending using autorun for sideeffects inside useEffect? It would re-render and run the effect anytime a tracked prop is changed ?

Or is it only props used in the render body and not inside the component?

danielkcz commented 4 years ago

@mweststrate

@FredyC can we update the docs? I think explaining the options as kinda equals just adds confusion. Let's make clear the recommendation is observer and <Observer> for the case that don't fit that. Personally I'm still doubtful we should expose useObserver as all, but it was important for one of your abstractions IIRC?

Apparently, people don't consider useObserver as a low level ... https://www.youtube.com/watch?v=pnhIJA64ByY

I used it sometimes too mainly because it's more convenient regarding the name of the component for debugging purposes. I don't have any abstractions, but @xaviergonz had one somewhere where he needed to use custom forceUpdate.

@seivan For a combination of useEffect & autorun, I usually wrap it into a custom hook which effectively removes ESLint ability to complain about empty deps.

The approach when you have observables in useEffect (3) works simply because the effect is executed after every render so it always sees the latest values. So yea, in that case, it's perfectly valid although not really efficient. With autorun you make sure that effect runs only when observed values change.

If you don't have further questions, please close the issue.

lock[bot] commented 4 years ago

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.