Useless deepEqual check in structuralSharing?

[Rubilmax]

Check existing issues

• I checked there isn't already an issue for the bug I encountered.

Describe the bug

I see this check in wagmi's default structuralSharing function:

wagmi/packages/core/src/query/utils.ts

Line 8 in 924cc93

if (deepEqual(oldData, newData)) return oldData as data

And I'm wondering if it's not useless, knowing that tanstack query's replaceEqualDeep already merge recursively merge objects the most referentially stable possible

In other words, replaceEqualDeep({ test: {}, k: 1 }, { test: {}, k: 2 }).test has the same reference as the first object's test property, thanks to this line

So what is the purpose of deepEqual in structuralSharing in this case?

I suppose it is a "performance" optimization check because deepEqual is faster than replaceEqualDeep, but is there any benchmark for this?
It seems to potentially be an additional non trivial deep equality check that is not worth the cost (because replaceEqualDeep will only replace references that are not deep equal)

Link to Minimal Reproducible Example

No response

Steps To Reproduce

const src = { test: {}, k: 1 };

expect(replaceEqualDeep(src, { test: {}, k: 2 }).test).toBe(src.test);

What Wagmi package(s) are you using?

@wagmi/core

Wagmi Package(s) Version(s)

2.12.16

Viem Version

2.21.8

TypeScript Version

5.6.2

Anything else?

No response

[Rubilmax]

So I just realized that it does not work with classes as the following test fails:

import { replaceEqualDeep } from "@tanstack/query-core";
import { describe, expect, test } from "vitest";

class Test {
  public readonly test = 1;
}

describe("structuralSharing", () => {
  test("should share the same object", () => {
    const prevTest = new Test();

    const newTest = new Test();

    expect(replaceEqualDeep(prevTest, newTest)).toBe(prevTest);
  });
});

Due to the isPlainObject check in tanstack query

But then, would we want wagmi to cover this edge case with a custom mergeDeepEqual function used in structuralSharing?

EDIT:

In the end, here is what I came up with to handle classes:

import { Market, MarketConfig } from "@morpho-org/blue-sdk";
import { replaceEqualDeep } from "@tanstack/query-core";
import { describe, expect, test } from "vitest";

export function isPlainArray(value: unknown) {
  return Array.isArray(value) && value.length === Object.keys(value).length;
}

/**
 * This function returns `a` if `b` is deeply equal.
 * If not, it will replace any deeply equal children of `b` with those of `a`.
 * This can be used for structural sharing between JSON values for example.
 * It may be unsafe to use with JS classes or other non-plain objects because it will not preserve the prototype chain.
 */
export function mergeDeepEqual<T>(a: unknown, b: T): T;
export function mergeDeepEqual(a: any, b: any): any {
  if (a === b) return a;

  if (
    a == null ||
    typeof a === "number" ||
    typeof a === "string" ||
    typeof a === "boolean" ||
    typeof a === "bigint" ||
    typeof a === "symbol"
  )
    return b;

  const array = isPlainArray(a) && isPlainArray(b);

  const aItems = array ? a : Object.keys(a);
  const aSize = aItems.length;
  const bItems = array ? b : Object.keys(b);
  const bSize = bItems.length;
  const copy = Object.create(
    Object.getPrototypeOf(a),
    Object.getOwnPropertyDescriptors(a),
  );

  let equalItems = 0;

  for (let i = 0; i < bSize; i++) {
    const key = array ? i : bItems[i];
    if (
      ((!array && aItems.includes(key)) || array) &&
      a[key] === undefined &&
      b[key] === undefined
    ) {
      copy[key] = undefined;
      equalItems++;
    } else {
      copy[key] = mergeDeepEqual(a[key], b[key]);
      if (copy[key] === a[key] && a[key] !== undefined) {
        equalItems++;
      }
    }
  }

  return aSize === bSize && equalItems === aSize ? a : copy;
}

const market = new Market({
  config: new MarketConfig({
    collateralToken: "0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0",
    irm: "0x870aC11D48B15DB9a138Cf899d20F13F79Ba00BC",
    lltv: 860000000000000000n,
    loanToken: "0xdAC17F958D2ee523a2206206994597C13D831ec7",
    oracle: "0x95DB30fAb9A3754e42423000DF27732CB2396992",
  }),
  fee: 0n,
  lastUpdate: 1727795543n,
  price: 2942555708084216647826084922n,
  rateAtTarget: 584469632n,
  totalBorrowAssets: 2977356497368n,
  totalBorrowShares: 2835315603261918002n,
  totalSupplyAssets: 3161520661952n,
  totalSupplyShares: 3030791088453168045n,
});

describe("structuralSharing", () => {
  test("tanstack query should not be optimal with classes", () => {
    const newMarket = new Market(market);

    expect(replaceEqualDeep(market, newMarket)).not.toBe(market);
  });

  test("mergeDeepEqual should be optimal with classes", () => {
    const newMarket = new Market(market);

    expect(mergeDeepEqual(market, newMarket)).toBe(market);
  });

  test("mergeDeepEqual should update reference if at least one reference changed", () => {
    const newMarket = new Market(market);

    newMarket.fee = 1n;

    const merged = mergeDeepEqual(market, newMarket);

    expect(merged).not.toBe(market);
    expect(merged.config).toBe(market.config);
  });
});

This may not be tanstack query friendly, but it handles classes as I want so far.

Still this issue's main topic remains: I believe deepEqual usage in structuralSharing is sub-optimal

[1997roylee]

I do think Wagmi is using tanquery as a default.

return useQuery({
    ...query,
    ...options,
    enabled,
    structuralSharing: query.structuralSharing ?? structuralSharing,
  })

[github-actions]

This issue has been locked since it has been closed for more than 14 days.

If you found a concrete bug or regression related to it, please open a new bug report with a reproduction against the latest Wagmi version. If you have any questions or comments you can create a new discussion thread.