mobxjs / mobx

Simple, scalable state management.
http://mobx.js.org
MIT License
27.56k stars 1.78k forks source link

reaction with previous value #1785

Closed xaviergonz closed 5 years ago

xaviergonz commented 6 years ago

Welcome to MobX. Please provide as much relevant information as possible!

I have a:

  1. [x] Idea:
    • [x] What problem would it solve for you?
    • [x] Do you think others will benefit from this change as well and it should in core package (see also mobx-utils)?
    • [x] Are you willing to (attempt) a PR yourself?
const noPreviousValue = Symbol()
reaction(() => ..., (newValue, previousValue /* noPreviousValue if no previous value*/) => {
})

sometimes the previous value is useful to make decisions on how to react to the change right now we have to do workarounds like

let oldValue = undefined;
reaction(() => ..., (newValue) => {
  // do something comparing new with old if needed
  oldValue = newValue
})

but it would be cool if it was built-in as part of reaction

if performance/garbage collection is an issue for some reason there could be an option such as { trackPreviousValue: boolean} // defaults to false but since in theory the previous value would be garbage collected as soon as the reaction is done running I don't see why it would be at first

xaviergonz commented 6 years ago

(lame) use case example, in a game when you want to show if a hit to your hp is critical or not

reaction(() => npc.hp, (newVal, oldVal) => {
  const hpLost = oldVal - newVal;
  if (hpLost > 100) { critical hit! }
  else { non critical hit }
});
urugator commented 6 years ago

The second param is already used for the reaction object itself, we could expose it as it's property:

reaction(() => npc.hp, (newVal, r) => {
  const hpLost = r.prevValue - newVal;
  if (hpLost > 100) { critical hit! }
  else { non critical hit }
});

However I think that the presented workaround is quite clean and easy to follow, so not sure if this makes it better, but I am also a bit biased against reactions so...

xaviergonz commented 6 years ago

OK, I think computed is usually a better option too, but how would I get the previous value of a computed and keep that previous value being observable? since computed relies on autorun it would be marked as dependant on its previous value and therefore keep recomputing itself... maybe using untracked?

basically what I mean is, would it make sense to add a previous value tracker to computed? something like '.getPreviousValue()' when some option is set

xaviergonz commented 6 years ago

or maybe add it as a parameter to the functional version const hpLost = computed((prev) => prev - this.hp)

xaviergonz commented 6 years ago

what about this then

hp = computed(() =>{return this.obshp}, {trackPrev:true} ) 
@computed get hpLost() {return  this.hp.getPrev() - this.hp.get()} 
xaviergonz commented 6 years ago

trackPrev could even take a number for how many previous values to keep and get Prev could have an optional index parameter to know how far back you want to go

xaviergonz commented 6 years ago

doesn't even need to be called computed, could be called trackedComputed and be part of mobx utils

urugator commented 6 years ago

The prev of computed is not always available, the cached value is just an optimization and and shoul be opaque imo. It could be possible with keepAlive though, or as you suggested via different api. Still... keeping the old value synchronized in hp setter, seems like an apporach a lot easier to follow...

@action setHp(hp) {
  this.prevHp = this.hp;
  this.hp = hp;
}
xaviergonz commented 6 years ago

yeah, but that's assuming you have ownership over the code of the observed object. imagine a react component where you want to track how a prop changes over time to decide what animation to play for example

mayorovp commented 6 years ago

@xaviergonz your example is bad. You assume that every hp change is a hit, but this assumption is false. Hp change may be two hits in one transaction. Or an enchanted equipment removal. Or a game loading.

@urugator animations cannot use previous value too: this behavior can cause visual glitches. For example, some value was changed from 1 to 3 and in the middle of animation that value has changed again to 4. Now we need to animate that value from 2 to 4 but not from 3 to 4.

mayorovp commented 6 years ago

@xaviergonz computed.observe already have previous value support and works like reaction:

mobx.computed(() => ...)
    .observe(({ newValue, oldValue }) => { 
        ... 
    });
xaviergonz commented 6 years ago

@mayorovp didn't think of using observe can it be really used like that (never saw it before), or is it

observe(someComputed, ({newValue, oldValue}) => {...})
mweststrate commented 6 years ago

@xaviergonz the intended behavior can indeed be achieved by using observe. I'm not sure whether the case is common enough to justify it being start of the standard api. If it is added, it should be using a flag indeed to avoid memory leaks.

xaviergonz commented 6 years ago

no worries, I can just use observe for now :)

Ciantic commented 5 years ago

I use this handy helper for reactions:

import { reaction } from "mobx";
import { isEqual } from "lodash";

export const reactionWithOldValue = <T>(
    expression: () => T,
    effect: (newValue: T, oldValue?: T) => void
) => {
    let oldValue: T;
    return reaction(expression, v => {
        if (!isEqual(v, oldValue)) {
            effect(v, oldValue);
            oldValue = v;
        }
    });
};

Note Implementation perhaps leaks memory, since the oldValue is not cleaned.

Usage:

let client = observable("");
reactionWithOldValue(
    () => client,
    (newValue, oldValue) => console.log(newValue, oldValue) // do something with old and new value
);
lock[bot] commented 5 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.