mobxjs / mobx-utils

Utility functions and common patterns for MobX
MIT License
1.19k stars 126 forks source link

deepObserve callback getting called multiple times within a single runInAction function #294

Open andrewvarga opened 3 years ago

andrewvarga commented 3 years ago

I have a simple case that looks something like this:


const myArray = observable([]);
deepObserve(myArray, () => {
   console.log("array update");
});

runInAction(() => {
   myArray.length = 1;
   myArray.length = 2;
});

The log in this case runs twice, once for each length modification, immediately after each. Is this normal? I would expect that because those length updates are in a runInAction, the observer only gets updated once, after the action is fully executed.

andrewvarga commented 3 years ago

Here is a jsfiddle link for anyone who'd like to try it out: https://jsfiddle.net/3jrpatdm/

mweststrate commented 3 years ago

This is working as intended, like observe, deepObserve reacts to all mutations, not to the resulting values, so transactions dont apply to them. Using observe should be very rarely needed and typically reaction/ autorun is the better solution although it might require a little mental shift.

On Sat, 13 Mar 2021, 10:17 Andrew, @.***> wrote:

Here is a jsfiddle link for anyone who'd like to try it out: https://jsfiddle.net/3jrpatdm/

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-utils/issues/294#issuecomment-798095461, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBB6AIKP23TTZUZSJ6TTDM3T5ANCNFSM4ZDEFW3Q .

andrewvarga commented 3 years ago

oh I see, I wasn't aware of this. My use case is that I simply want to call a function whenever my observed array changes (including changes within the elements of the array). Is there a way to do with autorun / reaction?

mweststrate commented 3 years ago

Yes. See the docs on those and the page about understanding what mobx reacts to.

On Sat, 13 Mar 2021, 13:02 Andrew, @.***> wrote:

oh I see, I wasn't aware of this. My use case is that I simply want to call a function whenever my observed array changes (including changes within the elements of the array). Is there a way to do with autorun / reaction?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-utils/issues/294#issuecomment-798339460, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBHAYLWSLGC7IXLKFGLTDNO65ANCNFSM4ZDEFW3Q .

andrewvarga commented 3 years ago

Thank you, I read that and played with those now. My understanding is that both autorun and reaction track the values that are accessed in their first parameter (effect / data function), so accessing the array itself will not cause the callback to run:

const myArray = observable([]);

reaction(() => myArray, () => {
   // This will not run in this example
   console.log("array update");
   document.getElementById("log").innerHTML += "array update<br>";
});

myArray.length = 2;
myArray.push({key: 5});

For me to get notified I need to access myArray.length in the first parameter function, but that is not enough if I also want to be notified about changes within the array elements (when myArray.length doesn't change). So I'd have to actually call my function in the first parameter, which accesses all internal values of the array..which in my use case is a bit problematic, because it's a very heavy function (it uses myArray to loop through a big data structure), ideally it would only be triggered once myArray changes, not any time before that, that's why deepObserve seemed like a good choice.

I can see 2 solutions:

Does that sound reasonable or did I miss anything? Thank you!

NaridaL commented 3 years ago

Your issue is you don't want your reaction to be called when you set up the autorun/reacion?

On Sun, 14 Mar 2021 at 23:01, Andrew @.***> wrote:

Thank you, I read that and played with those now. My understanding is that both autorun and reaction track the values that are accessed in their first parameter (effect / data function), so accessing the array itself will not cause the callback to run:

const myArray = observable([]); reaction(() => myArray, () => { // This will not run in this example console.log("array update"); document.getElementById("log").innerHTML += "array update
";}); myArray.length = 2;myArray.push({key: 5});

For me to get notified I need to access myArray.length in the first parameter function, but that is not enough if I also want to be notified about changes within the array elements (when myArray.length doesn't change). So I'd have to actually call my function in the first parameter, which accesses all internal values of the array..which in my use case is a bit problematic, because it's a very heavy function (it uses myArray to loop through a big data structure), ideally it would only be triggered once myArray changes, not any time before that, that's why deepObserve seemed like a good choice.

I can see 2 solutions:

  • create a less heavy data function that accesses all parts of myArray without going though the heavy data processing and use that for autorun / reaction
  • create a helper for deepObserve which "throttles" notifications per requestAnimationFrame, so updates will only be triggered at most once per frame.

Does that sound reasonable or did I miss anything? Thank you!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-utils/issues/294#issuecomment-798987938, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACSTYSRGA77QSYXKRO7RGCLTDUW4RANCNFSM4ZDEFW3Q .

andrewvarga commented 3 years ago

Yes, that would be very convenient, to just say "I want a callback to be invoked when any value within this observed object changes", and ideally only 1 invocation when the multiple things change in the observed object within an action.

But those 2 workarounds I mentioned can work too, I think, if there is no other way.

mweststrate commented 3 years ago

I think you might still missing the point of autorun, as what you describe is what it is supposed to do; you run an effect and during that it merely administers what is read as that is relevant for a next run. I recommend to read the pages about the philosophy and gist of MobX, and/or setup a sandbox that clarifies why it doesn't work for your case

On Sun, 14 Mar 2021, 22:44 Andrew, @.***> wrote:

Yes, that would be very convenient, to just say "I want a callback to be invoked when any value within this observed object changes", and ideally only 1 invocation when the multiple things change in the observed object within an action.

But those 2 workarounds I mentioned can work too, I think, if there is no other way.

Your issue is you don't want your reaction to be called when you set up the autorun/reacion? … <#m-6930870244993986151> On Sun, 14 Mar 2021 at 23:01, Andrew @.***> wrote: Thank you, I read that and played with those now. My understanding is that both autorun and reaction track the values that are accessed in their first parameter (effect / data function), so accessing the array itself will not cause the callback to run: const myArray = observable([]); reaction(() => myArray, () => { // This will not run in this example console.log("array update"); document.getElementById("log").innerHTML += "array update ";}); myArray.length = 2;myArray.push({key: 5}); For me to get notified I need to access myArray.length in the first parameter function, but that is not enough if I also want to be notified about changes within the array elements (when myArray.length doesn't change). So I'd have to actually call my function in the first parameter, which accesses all internal values of the array..which in my use case is a bit problematic, because it's a very heavy function (it uses myArray to loop through a big data structure), ideally it would only be triggered once myArray changes, not any time before that, that's why deepObserve seemed like a good choice. I can see 2 solutions: - create a less heavy data function that accesses all parts of myArray without going though the heavy data processing and use that for autorun / reaction - create a helper for deepObserve which "throttles" notifications per requestAnimationFrame, so updates will only be triggered at most once per frame. Does that sound reasonable or did I miss anything? Thank you! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#294 (comment) https://github.com/mobxjs/mobx-utils/issues/294#issuecomment-798987938>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACSTYSRGA77QSYXKRO7RGCLTDUW4RANCNFSM4ZDEFW3Q .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-utils/issues/294#issuecomment-798993949, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBD725WVY3U4VMSDYADTDU35RANCNFSM4ZDEFW3Q .

andrewvarga commented 3 years ago

I read those pages and they seem clear to me. My concern is that my effect is a very heavy function that I only want to be invoked once the data actually changes, and not initially for initializing autorun or reaction. Here is a hopefully better example: https://jsfiddle.net/xfudacon/2/

You can see that the heavy function is triggered 2x. The second one is in response to changing the myFilters array so that's fine, but it's also running the first time, looping through all elements in myData. So to avoid the heavy function to be called twice, I still think these are the 2 options:

mweststrate commented 3 years ago

I'm not sure what heavy looks like, but calling heavy n times versus n+1 times is the same problem from a cost perspective. Either the function is unbearably heavy and locking up the app, or it isn't. But the +1 isn't going to make the fundamental difference and I rather look into optimizing heavy itself then. For example you might memoize the filter result for existing entries, and only apply filtering to your newly incoming data instead of the whole set, etc. But so far it doesn't sound your 1 run less is going to make the difference, and there is probably a bigger underlying problem; heavy being heavy.

Note that autorun does support a custom scheduler, in case you want to debounce it still, even after respecting transactions.

On Mon, Mar 15, 2021 at 9:58 AM Andrew @.***> wrote:

I read those pages and they seem clear to me. My concern is that my effect is a very heavy function that I only want to be invoked once the data actually changes, and not initially for initializing autorun or reaction. Here is a hopefully better example: https://jsfiddle.net/xfudacon/2/

You can see that the heavy function is triggered 2x. The second one is in response to changing the myFilters array so that's fine, but it's also running the first time, looping through all elements in myData. So to avoid the heavy function to be called twice, I still think these are the 2 options:

  • use a reaction and create a lightweight data accessor function which is ok to be called initially. In this example it would be trivial to do this but in my actual code the heavy function is quite long and uses a complex filter object, so it's not as straightforward although not impossible either
  • use a throttled deepObserve although that feels like a hack

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-utils/issues/294#issuecomment-799285758, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBBBAGM7SVLPEVSALS3TDXK5PANCNFSM4ZDEFW3Q .

andrewvarga commented 3 years ago

In this case, I think it matters. The list of items to go through can be in 10s of thousands and for each item the filter array needs to be looped through too. That stalls the UI for seconds and it makes a huge difference to do this only once or twice initially. Like 4 seconds or 8 seconds, both of them being too much of course, I agree. This heavy operation itself is memoized, it is only running again once the large dataset or the filters change.

But debouncing seems like a good option to have too, I'll try and see what I can do with that. Thanks for the help.

mweststrate commented 3 years ago

sounds like you'd really benefit from splitting up the computation into many small async tasks :). But yeah that is definitely beyond the scope of this project

On Wed, Mar 17, 2021 at 10:16 PM Andrew @.***> wrote:

In this case, I think it matters. The list of items to go through can be in 10s of thousands and for each item the filter array needs to be looped through too. That stalls the UI for seconds and it makes a huge difference to do this only once or twice initially. Like 4 seconds or 8 seconds, both of them being too much of course, I agree. This heavy operation itself is memoized, it is only running again once the large dataset or the filters change.

But debouncing seems like a good option to have too, I'll try and see what I can do with that. Thanks for the help.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-utils/issues/294#issuecomment-801477894, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBHM6VKOOQXHTWMRVJDTEES5HANCNFSM4ZDEFW3Q .

dested commented 2 years ago

Ran across this issue today. The reasoning makes sense but it still threw me for a bit of a loop. For any future googlers, I added a simple debounce to my deepObserve callback and that worked for my usecase.