Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maximum call stack exceeded with salePrice ref (video) #2

Open
uturnr opened this issue Sep 21, 2020 · 5 comments · May be fixed by #4
Open

Maximum call stack exceeded with salePrice ref (video) #2

uturnr opened this issue Sep 21, 2020 · 5 comments · May be fixed by #4

Comments

@uturnr
Copy link

uturnr commented Sep 21, 2020

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.

@Eugene930
Copy link

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

@MellowCobra
Copy link

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
Copy link

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
}

@Eugene930
Copy link

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.

@BorislavBorisov22
Copy link

BorislavBorisov22 commented Apr 19, 2021

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);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants