vue-3-reactivity icon indicating copy to clipboard operation
vue-3-reactivity copied to clipboard

Maximum call stack exceeded with salePrice ref (video)

Open uturnr opened this issue 4 years ago • 5 comments

In the course video (but not the text below), the salePrice effect is declared after the total effect. This causes an infinite loop.

(Example of order in video)

effect(() => {
  total = salePrice.value * product.quantity
})
effect(() => {
  salePrice.value = product.price * 0.9
})

I added a check in the ref function to not set the value if it had not changed, which solved it for me:

    set value(newVal) {
      if (newVal !== raw) {
        raw = newVal
        trigger(r, 'value')
      }
    }

But then I came here to investigate and I noticed the difference, so I thought I'd point it out.

uturnr avatar Sep 21 '20 23:09 uturnr

Same problem, and I find out that it's because the trigger function do not change the activeEffect variable when there is a nested trigger, problem code here;

So I make some change in the trigger function:

dep.forEach((innerEffect) => {
  effect(innerEffect);
});

this change the activeEffect variable when there is a deeper trigger

Eugene930 avatar Oct 03 '20 11:10 Eugene930

Glad I'm not the only one who ran into this issue! Both of the above look like good solutions. I went ahead and put them into my own notes in case I come back to look at this later. Thanks for bringing this up!

MellowCobra avatar Nov 30 '20 17:11 MellowCobra

Following @Eugene930's example, should we also change the effect function? Currently in the tutorials, it sets activeEffect to null at the end of the function. This seems like if we add support for nested effect calls, this might be problematic as an inner effect() call will completely clear out the parent's effect.

Should it instead temporarily replace the active effect and then re-place it when it is done? Such as the following:

function effect(eff) {
  const prevEffect = activeEffect
  activeEffect = eff
  activeEffect()
  activeEffect = prevEffect
}

MellowCobra avatar Nov 30 '20 17:11 MellowCobra

Following @Eugene930's example, should we also change the effect function? Currently in the tutorials, it sets activeEffect to null at the end of the function. This seems like if we add support for nested effect calls, this might be problematic as an inner effect() call will completely clear out the parent's effect.

Should it instead temporarily replace the active effect and then re-place it when it is done? Such as the following:

function effect(eff) {
  const prevEffect = activeEffect
  activeEffect = eff
  activeEffect()
  activeEffect = prevEffect
}

Yes we should change. I think you could take a look at vue3's reactivity repo, they use a better way: stack , to achieve that.

Eugene930 avatar Dec 02 '20 05:12 Eugene930

I was just about to make a pr for a fix that I came up with, then I saw that there is already an issue about this. I simply introduced a new variable skipTrack and set it to true before trigger and then back to false once we have triggered the dep effects because I was not able to think of a case where we want to track any deps while triggering a setter.

function reactivity() {
  const targetMap = new WeakMap();
  let activeEffect = null;
  let skipTrack = false;

  function effect(eff) {
    activeEffect = eff;
    activeEffect();
    activeEffect = null;
  }

  function track(target, key) {
    if (skipTrack || !activeEffect) {
      return;
    }

    let depsMap = targetMap.get(target);
    if (!depsMap) {
      targetMap.set(target, (depsMap = new Map()));
    }

    let dep = depsMap.get(key);
    if (!dep) {
      depsMap.set(key, (dep = new Set()));
    }

    dep.add(activeEffect);
  }

  function runEffects(dep) {
    skipTrack = true;
    dep.forEach(eff => eff());
    skipTrack = false;
  }

  function trigger(target, key) {
    const depsMap = targetMap.get(target);
    if (!depsMap) {
      return;
    }

    const dep = depsMap.get(key);
    if (!dep) {
      return;
    }

    runEffects(dep);
  }

  function reactive(target) {
    const handler = {
      get(target, key, receiver) {
        const result = Reflect.get(target, key, receiver);
        track(target, key);

        return result;
      },
      set(target, key, value, receiver) {
        const oldValue = target[key];
        const result = Reflect.set(target, key, value, receiver);
        if (result && oldValue !== value) {
          trigger(target, key);
        }

        return result;
      }
    }

    return new Proxy(target, handler);
  }

  function ref(raw) {
    const r = {
      get value() {
        track(r, 'value');
        return raw;
      },
      set value(newValue) {
        const oldValue = raw;
        raw = newValue;
        if (newValue !== oldValue) {
          trigger(r, 'value');
        }
      }
    }

    return r;
  }

  function computed(getter) {
    let result = ref(null);
    effect(() => {
      result.value = getter();
    });

    return result;
  }

  return { effect, reactive, ref, computed, };
}

const { effect, reactive, ref, computed } = reactivity();

const product = reactive({ price: 10, quantity: 2 });
const salePrice = ref(0);
let total = 0;

console.log(total, salePrice.value);

BorislavBorisov22 avatar Apr 19 '21 20:04 BorislavBorisov22