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

Issue with @signal's Cache: computed Instances Sharing the Same Cache #91

Closed
JuerGenie opened this issue Dec 12, 2024 · 2 comments
Closed

Comments

@JuerGenie
Copy link
Contributor

As shown in the screenshots, the computed function is only called when the first instance is created and updated, but not for other instances:

1733986345960
1733986360823

⬆️ All content looks the same (they are the first instance's content).

The issue is caused by the following code:

// file: signal-utils/src/index.ts

function computedDecorator<Value = any>(
  target: () => Value,
  context: ClassGetterDecoratorContext,
): () => Value {
  /* ... */

  let caches = new WeakMap<typeof target, Signal.Computed<Value>>(); // <- caches target to `Computed` instance, not instance.

  return function (this: unknown) {
    let cache = caches.get(target); // <- always gets the same cache regardless of `this`.
    /* ... */
  };
}

This is my solution:

function computedDecorator<Value = any>(
  target: () => Value,
  context: ClassGetterDecoratorContext,
): () => Value {
  /* ... */

  let caches = new WeakMap<typeof target, WeakMap<object, Signal.Computed<Value>>>();

  return function (this: unknown) {
    let cache = caches.get(target);
    if (!cache) {
      cache = new WeakMap();
      caches.set(target, cache);
    }
    let effect = cache.get(this as object);
    if (!effect) {
      effect = new Signal.Computed(() => target.call(this));
      cache.set(this as object, effect);
    }

    return effect.get();
  };
}

It's work for me.

@aomarks
Copy link
Contributor

aomarks commented Dec 23, 2024

I can confirm that @JuerGenie's patch above fixed some gnarly bugs I was hitting involving the @signal decorator sharing cached values in very mysterious ways. Thank you!

It would be great to get the fix landed -- @JuerGenie any chance you might send your patch as a PR? :)

@JuerGenie
Copy link
Contributor Author

I can confirm that @JuerGenie's patch above fixed some gnarly bugs I was hitting involving the @signal decorator sharing cached values in very mysterious ways. Thank you!

It would be great to get the fix landed -- @JuerGenie any chance you might send your patch as a PR? :)

Sure, I'll open a PR later today :D

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

No branches or pull requests

2 participants