Cache params ordering issue

.changeset/brown-colts-refuse.md

@@ -0,0 +1,5 @@
+---
+'@farfetched/core': patch
+---
+
+Decreased number of cache misses

.changeset/ya-naebal-random.md

@@ -0,0 +1,5 @@
+---
+'@farfetched/core': patch
+---
+
+Fixed bug with trying to cache unserializable params https://github.com/igorkamyshev/farfetched/issues/156

packages/core/src/cache/__test__/cache.test.ts

@@ -204,7 +204,7 @@ describe('cache', () => {
 
     // Do not await
     allSettled(query.start, { scope });
-    // But wait for next tick becuase of async adapter's nature
+    // But wait for next tick because of async adapter's nature
     await setTimeout(1);
 
     // Value from cache not found
@@ -220,6 +220,49 @@ describe('cache', () => {
     expect(handler).toBeCalledTimes(2);
   });
 
+  test('works with non-serializable params', async () => {
+    const logSpy = vi
+      .spyOn(global.console, 'warn')
+      // eslint-disable-next-line @typescript-eslint/no-empty-function
+      .mockImplementation(() => {});
+    let index = 1;
+
+    const mockFn = vi.fn();
+
+    const handler = vi.fn(async (_: typeof mockFn) =>
+      setTimeout(1000).then(() => index++)
+    );
+
+    const query = withFactory({ fn: () => createQuery({ handler }), sid: '1' });
+
+    cache(query);
+
+    const scope = fork();
+
+    await allSettled(query.start, { scope, params: mockFn });
+
+    expect(scope.getState(query.$data)).toEqual(1);
+
+    await allSettled(query.reset, { scope });
+
+    // Do not await
+    allSettled(query.start, { scope, params: mockFn });
+    // But wait for next tick because of async adapter's nature
+    await setTimeout(1);
+
+    // Value from cache
+    expect(scope.getState(query.$data)).toEqual(null);
+    expect(scope.getState(query.$stale)).toBeFalsy();
+
+    await allPrevSettled(scope);
+
+    // After refetch, it's new value
+    expect(scope.getState(query.$data)).toEqual(2);
+    expect(scope.getState(query.$stale)).toBeFalsy();
+
+    expect(handler).toBeCalledTimes(2);
+  });
+
   test('ignore null-key', async () => {
     // params cannot be serialized
     const params: any = {};

packages/core/src/cache/key/__test__/key.no_sources.test.ts

@@ -97,6 +97,38 @@ describe('key, without sourced', () => {
     expect(firstArg(listener, 0).key).toBe(firstArg(listener, 1).key);
   });
 
+  test('same keys in different order for same params', async () => {
+    const query = withFactory({
+      fn: () =>
+        createQuery({
+          handler: async (p: { a: string; b: { values: number[] } }) => 1,
+        }),
+      sid: '1',
+    });
+
+    const startWithKey = enrichStartWithKey(query);
+
+    const listener = vi.fn();
+
+    startWithKey.watch(listener);
+
+    const scope = fork();
+
+    await allSettled(query.start, {
+      scope,
+      params: { a: 'str', b: { values: [1, 2] } },
+    });
+    await allSettled(query.start, {
+      scope,
+      params: { b: { values: [1, 2] }, a: 'str' },
+    });
+
+    expect(firstArg(listener, 0).key.length).toBeGreaterThanOrEqual(1);
+    expect(firstArg(listener, 1).key.length).toBeGreaterThanOrEqual(1);
+
+    expect(firstArg(listener, 0).key).toBe(firstArg(listener, 1).key);
+  });
+
   test('same keys for no params', async () => {
     const query = withFactory({
       fn: () => createQuery({ handler: async (p: void) => 1 }),

packages/core/src/cache/key/key.ts

@@ -6,6 +6,7 @@ import {
   RemoteOperationParams,
 } from '../../remote_operation/type';
 import { sha1 } from '../lib/hash';
+import { stableStringify } from '../lib/stable_stringify';
 
 export function enrichFinishedSuccessWithKey<Q extends Query<any, any, any>>(
   query: Q
@@ -52,8 +53,10 @@ function createKey({
   sources: unknown[];
 }): string | null {
   try {
-    return sha1(sid + JSON.stringify(params) + JSON.stringify(sources));
-  } catch (e) {
+    const stableString = stableStringify({ params, sources, sid })!;
+
+    return sha1(stableString);
+  } catch (e: unknown) {
     return null;
   }
 }

packages/core/src/cache/lib/__test__/stable_stringify.spec.ts

@@ -0,0 +1,127 @@
+import { describe, test, expect } from 'vitest';
+
+import { stableStringify } from '../stable_stringify';
+
+function expectWorkAsDefaultJSONSerializer(obj: unknown) {
+  const stableStr = stableStringify(obj)!;
+  const jsonStr = JSON.stringify(obj);
+
+  expect(JSON.parse(stableStr)).toEqual(JSON.parse(jsonStr));
+}
+
+describe('stableStringify', () => {
+  test('simple object', () => {
+    const obj = { c: 6, b: [4, 5], a: 3, z: null };
+    const anotherOrderObj = { b: [4, 5], c: 6, z: null, a: 3 };
+
+    expect(stableStringify(obj)).toEqual('{"a":3,"b":[4,5],"c":6,"z":null}');
+    expect(stableStringify(anotherOrderObj)).toEqual(stableStringify(obj));
+    expectWorkAsDefaultJSONSerializer(obj);
+  });
+
+  test('object with undefined', () => {
+    const obj = { a: 3, b: undefined };
+
+    expect(stableStringify(obj)).toEqual('{"a":3}');
+    expectWorkAsDefaultJSONSerializer(obj);
+  });
+
+  test('object with null', () => {
+    const obj = { a: 3, b: null };
+
+    expect(stableStringify(obj)).toEqual('{"a":3,"b":null}');
+    expectWorkAsDefaultJSONSerializer(obj);
+  });
+
+  test('object with empty string', () => {
+    const obj = { a: 3, z: '' };
+
+    expect(stableStringify(obj)).toEqual('{"a":3,"z":""}');
+    expectWorkAsDefaultJSONSerializer(obj);
+  });
+
+  test('object with NaN and Infinity', () => {
+    const obj = { a: 3, b: NaN, c: Infinity };
+
+    expect(stableStringify(obj)).toEqual('{"a":3,"b":null,"c":null}');
+    expectWorkAsDefaultJSONSerializer(obj);
+  });
+
+  test('array with empty string', () => {
+    const obj = [4, '', 6];
+
+    expect(stableStringify(obj)).toEqual('[4,"",6]');
+    expectWorkAsDefaultJSONSerializer(obj);
+  });
+
+  test('array with undefined', () => {
+    const obj = [4, undefined, 6];
+
+    expect(stableStringify(obj)).toEqual('[4,null,6]');
+    expectWorkAsDefaultJSONSerializer(obj);
+  });
+
+  test('array with NaN and Infinity', () => {
+    const obj = [4, NaN, Infinity, 6];
+
+    expect(stableStringify(obj)).toEqual('[4,null,null,6]');
+    expectWorkAsDefaultJSONSerializer(obj);
+  });
+
+  test('nested object', () => {
+    const obj = { c: 8, b: [{ z: 6, y: 5, x: 4 }, 7], a: 3 };
+
+    expect(stableStringify(obj)).toEqual(
+      '{"a":3,"b":[{"x":4,"y":5,"z":6},7],"c":8}'
+    );
+    expectWorkAsDefaultJSONSerializer(obj);
+  });
+
+  test('throws an error for function', () => {
+    // eslint-disable-next-line @typescript-eslint/no-empty-function
+    const fn = () => {};
+
+    expect(() => stableStringify(fn)).toThrowError(`Can't serialize function`);
+    expect(() => stableStringify({ fn })).toThrowError(
+      `Can't serialize function`
+    );
+  });
+
+  test('throws an error for cycle dependency in object', () => {
+    const one: any = { a: 1 };
+    const two = { a: 2, one: one };
+
+    one.two = two;
+
+    expect(() => stableStringify(one)).toThrowError(
+      `Can't serialize cyclic structure`
+    );
+  });
+
+  test('throws an error for cycle dependency in array', () => {
+    const one: any[] = [1, 2];
+    const two = [3, 4, one];
+
+    one.push(two);
+
+    expect(() => stableStringify(one)).toThrowError(
+      `Can't serialize cyclic structure`
+    );
+  });
+
+  test('repeated non-cyclic value for object', () => {
+    const one = { x: 1 };
+    const two = { a: one, b: one };
+
+    expect(stableStringify(two)).toEqual('{"a":{"x":1},"b":{"x":1}}');
+    expectWorkAsDefaultJSONSerializer(two);
+  });
+
+  test('repeated non-cyclic value for array', () => {
+    const one = [1, 2, 3];
+    const two = [one, 2, one];
+
+    expect(stableStringify(two)).toEqual('[[1,2,3],2,[1,2,3]]');
+    expectWorkAsDefaultJSONSerializer(two);
+  });
+});

packages/core/src/cache/lib/stable_stringify.ts

@@ -0,0 +1,49 @@
+export function stableStringify(data: unknown): string | void {
+  const seen = new Set<unknown>();
+
+  function stringify(node: unknown): string | void {
+    if (node === undefined) return;
+    if (node === null) return 'null';
+
+    if (typeof node === 'number') {
+      return isFinite(node) ? `${node}` : 'null';
+    }
+
+    if (typeof node === 'function') {
+      throw new TypeError(`Can't serialize function`);
+    }
+
+    if (typeof node !== 'object') return JSON.stringify(node);
+
+    if (seen.has(node)) {
+      throw new TypeError(`Can't serialize cyclic structure`);
+    }
+
+    seen.add(node);
+
+    if (Array.isArray(node)) {
+      const values = node.map((v) => stringify(v) || 'null').join(',');
+
+      seen.delete(node);
+
+      return `[${values}]`;
+    }
+
+    const values = Object.keys(node)
+      .sort()
+      .map((key) => {
+        // @ts-expect-error We're working with unknown object
+        const value = stringify(node[key]);
+
+        return value ? `${stringify(key)}:${value}` : '';
+      })
+      .filter(Boolean)
+      .join(',');
+
+    seen.delete(node);
+
+    return `{${values}}`;
+  }
+
+  return stringify(data);
+}