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

Make computedFn return type stricter by forbiding non declared parameters #327

Open
jraoult opened this issue Feb 23, 2024 · 0 comments
Open

Comments

@jraoult
Copy link
Contributor

jraoult commented Feb 23, 2024

I've just discovered the source of a bug in my app caused by the loss of referential equality.

Here's the pseudo-code:

const memoizedMap = computedFn((shape) => new Facade(shape))

const prevFacades = shapes.map(memoizedMap);

// reverse to change index but not values
shapes. reverse();
// reverse back to original order
shapes. reverse();

const nextFacades = shapes.map(memoizedMap);

// throws, but I expected it to keep referential equality 
assert(prevFacades[0] === nextFacades[0])

It is because the memoised function is called with:

memoizedMap(shape, index, shapes)

So even if the shape is the same, the index is included in the cache key and requires re-computation of the value.

Now I realise it is my fault for not following that rule: Don't use functions as callbacks unless they're designed for it but I also thought having stricter type could help, so I implemented this patch to reject extra arguments:

diff --git a/lib/computedFn.d.ts b/lib/computedFn.d.ts
index 247a86b0a891b34272807922f6ae259f0afedb81..f1e8dd1d57830b6b40ddf6503216685674249a45 100644
--- a/lib/computedFn.d.ts
+++ b/lib/computedFn.d.ts
@@ -36,4 +36,12 @@ export declare type IComputedFnOptions<F extends (...args: any[]) => any> = {
  * @param fn
  * @param keepAliveOrOptions
  */
-export declare function computedFn<T extends (...args: any[]) => any>(fn: T, keepAliveOrOptions?: IComputedFnOptions<T> | boolean): T;
+export declare function computedFn<T extends (...args: any[]) => any>(
+  fn: T,
+  keepAliveOrOptions?: IComputedFnOptions<T> | boolean,
+): (
+  ...args: [
+    ...Parameters<T>,
+    ...forbidden: "computedFn should not take undeclared parameters to avoid unintended cache misses"[],
+  ]
+) => ReturnType<T>;

With this shapes.map(memoizedMap); would be enforce passing the arguments explicitly

shapes.map((shape) = > memoizedMap(shape));

Would you consider adding this?

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

1 participant