From 8ccf41d28576159d213c1edeeaa4bbaf4afdf630 Mon Sep 17 00:00:00 2001 From: Weil Liao Date: Tue, 27 Aug 2024 20:56:04 +0800 Subject: [PATCH 1/4] fix: prevent component re-rendering with the same account snapshot --- .../hooks/useSyncExternalStoreWithTracked.ts | 46 ++++++++++--------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/packages/react/src/hooks/useSyncExternalStoreWithTracked.ts b/packages/react/src/hooks/useSyncExternalStoreWithTracked.ts index d81afc76e2..4e372a556d 100644 --- a/packages/react/src/hooks/useSyncExternalStoreWithTracked.ts +++ b/packages/react/src/hooks/useSyncExternalStoreWithTracked.ts @@ -1,7 +1,7 @@ 'use client' import { deepEqual } from '@wagmi/core/internal' -import { useRef } from 'react' +import { useMemo, useRef } from 'react' import { useSyncExternalStoreWithSelector } from 'use-sync-external-store/shim/with-selector.js' const isPlainObject = (obj: unknown) => @@ -37,29 +37,31 @@ export function useSyncExternalStoreWithTracked< }, ) - if (isPlainObject(result)) { - const trackedResult = { ...result } - let properties = {} - for (const [key, value] of Object.entries( - trackedResult as { [key: string]: any }, - )) { - properties = { - ...properties, - [key]: { - configurable: false, - enumerable: true, - get: () => { - if (!trackedKeys.current.includes(key)) { - trackedKeys.current.push(key) - } - return value + return useMemo(() => { + if (isPlainObject(result)) { + const trackedResult = { ...result } + let properties = {} + for (const [key, value] of Object.entries( + trackedResult as { [key: string]: any }, + )) { + properties = { + ...properties, + [key]: { + configurable: false, + enumerable: true, + get: () => { + if (!trackedKeys.current.includes(key)) { + trackedKeys.current.push(key) + } + return value + }, }, - }, + } } + Object.defineProperties(trackedResult, properties) + return trackedResult } - Object.defineProperties(trackedResult, properties) - return trackedResult - } - return result + return result + }, [result]) } From 1eca9c73f2eddfdd2df4fe6c9605b1d8459ed369 Mon Sep 17 00:00:00 2001 From: Weil Liao Date: Fri, 27 Sep 2024 14:51:05 +0800 Subject: [PATCH 2/4] test: check component re-rendering caused by useSyncExternalStoreWithTracked --- ... useSyncExternalStoreWithTracked.test.tsx} | 50 ++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) rename packages/react/src/hooks/{useSyncExternalStoreWithTracked.test.ts => useSyncExternalStoreWithTracked.test.tsx} (72%) diff --git a/packages/react/src/hooks/useSyncExternalStoreWithTracked.test.ts b/packages/react/src/hooks/useSyncExternalStoreWithTracked.test.tsx similarity index 72% rename from packages/react/src/hooks/useSyncExternalStoreWithTracked.test.ts rename to packages/react/src/hooks/useSyncExternalStoreWithTracked.test.tsx index bf600a457b..30d08321e7 100644 --- a/packages/react/src/hooks/useSyncExternalStoreWithTracked.test.ts +++ b/packages/react/src/hooks/useSyncExternalStoreWithTracked.test.tsx @@ -1,4 +1,6 @@ -import { act, cleanup, renderHook } from '@wagmi/test/react' +import { fireEvent, screen } from '@testing-library/react' +import { act, cleanup, render, renderHook } from '@wagmi/test/react' +import React, { memo, useState } from 'react' import * as ReactDOM from 'react-dom' import { afterEach, describe, expect, it } from 'vitest' @@ -173,4 +175,50 @@ describe('useSyncExternalStoreWithTracked', () => { ] `) }) + + it("create store's new object reference when only values changed", async () => { + const externalStore = createExternalStore({ + foo: 'bar', + gm: 'wagmi', + isGonnaMakeIt: false, + }) + + let rerenders = -1 + + const TRIGGER_BTN_TEXT = 'Trigger re-render' + + const MemoComponent = memo((props: { store: any }) => { + rerenders++ + return
{props.store.isGonnaMakeIt}
+ }) + + const Test = () => { + const store = useExternalStore(externalStore, () => {}) + const [, forceTestRerender] = useState(0) + + return ( + <> + + + + ) + } + + render() + const forceRerenderBtn = screen.getByText(TRIGGER_BTN_TEXT) + + expect(rerenders).toBe(0) + + fireEvent.click(forceRerenderBtn) // updating other state won't make the store be a new object + + expect(rerenders).toBe(0) + + act(() => { + externalStore.set((x) => ({ ...x, isGonnaMakeIt: true })) // trigger rerendering when the used value changes + }) + + expect(rerenders).toBe(1) + }) }) From 1e4b6c0630c92e8b172c6e2c44bd0d882960c407 Mon Sep 17 00:00:00 2001 From: Tom Meagher Date: Fri, 27 Sep 2024 11:00:12 -0400 Subject: [PATCH 3/4] test: update --- .../useSyncExternalStoreWithTracked.test.tsx | 210 +++++++++--------- 1 file changed, 105 insertions(+), 105 deletions(-) diff --git a/packages/react/src/hooks/useSyncExternalStoreWithTracked.test.tsx b/packages/react/src/hooks/useSyncExternalStoreWithTracked.test.tsx index 30d08321e7..c806e1298a 100644 --- a/packages/react/src/hooks/useSyncExternalStoreWithTracked.test.tsx +++ b/packages/react/src/hooks/useSyncExternalStoreWithTracked.test.tsx @@ -1,8 +1,8 @@ import { fireEvent, screen } from '@testing-library/react' import { act, cleanup, render, renderHook } from '@wagmi/test/react' -import React, { memo, useState } from 'react' +import React from 'react' import * as ReactDOM from 'react-dom' -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, expect, test } from 'vitest' import { useSyncExternalStoreWithTracked } from './useSyncExternalStoreWithTracked.js' @@ -41,33 +41,32 @@ function useExternalStore( return state as any } -describe('useSyncExternalStoreWithTracked', () => { - afterEach(() => { - cleanup() - }) +afterEach(() => { + cleanup() +}) - it('rerenders only when the tracked value changes', async () => { - const externalStore = createExternalStore({ - foo: 'bar', - gm: 'wagmi', - isGonnaMakeIt: false, - }) +test('rerenders only when the tracked value changes', async () => { + const externalStore = createExternalStore({ + foo: 'bar', + gm: 'wagmi', + isGonnaMakeIt: false, + }) - const renders: any[] = [] + const renders: any[] = [] - renderHook(() => { - const { gm } = useExternalStore(externalStore, (state) => { - renders.push(state) - }) - - return gm + renderHook(() => { + const { gm } = useExternalStore(externalStore, (state) => { + renders.push(state) }) - act(() => { - externalStore.set((x) => ({ ...x, foo: 'baz', isGonnaMakeIt: true })) - }) + return gm + }) + + act(() => { + externalStore.set((x) => ({ ...x, foo: 'baz', isGonnaMakeIt: true })) + }) - expect(renders).toMatchInlineSnapshot(` + expect(renders).toMatchInlineSnapshot(` [ { "foo": "bar", @@ -77,11 +76,11 @@ describe('useSyncExternalStoreWithTracked', () => { ] `) - act(() => { - externalStore.set((x) => ({ ...x, gm: 'ngmi' })) - }) + act(() => { + externalStore.set((x) => ({ ...x, gm: 'ngmi' })) + }) - expect(renders).toMatchInlineSnapshot(` + expect(renders).toMatchInlineSnapshot(` [ { "foo": "bar", @@ -95,37 +94,37 @@ describe('useSyncExternalStoreWithTracked', () => { }, ] `) - }) +}) - it('rerenders when all values are being tracked', async () => { - const externalStore = createExternalStore({ - foo: 'bar', - gm: 'wagmi', - isGonnaMakeIt: false, - }) +test('rerenders when all values are being tracked', async () => { + const externalStore = createExternalStore({ + foo: 'bar', + gm: 'wagmi', + isGonnaMakeIt: false, + }) - const renders: any[] = [] + const renders: any[] = [] - renderHook(() => { - const { foo, gm, isGonnaMakeIt } = useExternalStore( - externalStore, - (state) => { - renders.push(state) - }, - ) + renderHook(() => { + const { foo, gm, isGonnaMakeIt } = useExternalStore( + externalStore, + (state) => { + renders.push(state) + }, + ) - return { - foo, - gm, - isGonnaMakeIt, - } - }) + return { + foo, + gm, + isGonnaMakeIt, + } + }) - act(() => { - externalStore.set((x) => ({ ...x, isGonnaMakeIt: true })) - }) + act(() => { + externalStore.set((x) => ({ ...x, isGonnaMakeIt: true })) + }) - expect(renders).toMatchInlineSnapshot(` + expect(renders).toMatchInlineSnapshot(` [ { "foo": "bar", @@ -139,28 +138,28 @@ describe('useSyncExternalStoreWithTracked', () => { }, ] `) - }) +}) - it('rerenders when no values are being tracked', async () => { - const externalStore = createExternalStore({ - foo: 'bar', - gm: 'wagmi', - isGonnaMakeIt: false, - }) +test('rerenders when no values are being tracked', async () => { + const externalStore = createExternalStore({ + foo: 'bar', + gm: 'wagmi', + isGonnaMakeIt: false, + }) - const renders: any[] = [] + const renders: any[] = [] - renderHook(() => { - useExternalStore(externalStore, (state) => { - renders.push(state) - }) + renderHook(() => { + useExternalStore(externalStore, (state) => { + renders.push(state) }) + }) - act(() => { - externalStore.set((x) => ({ ...x, isGonnaMakeIt: true })) - }) + act(() => { + externalStore.set((x) => ({ ...x, isGonnaMakeIt: true })) + }) - expect(renders).toMatchInlineSnapshot(` + expect(renders).toMatchInlineSnapshot(` [ { "foo": "bar", @@ -174,51 +173,52 @@ describe('useSyncExternalStoreWithTracked', () => { }, ] `) - }) +}) - it("create store's new object reference when only values changed", async () => { - const externalStore = createExternalStore({ - foo: 'bar', - gm: 'wagmi', - isGonnaMakeIt: false, - }) +test('store object reference is stable across rerenders', async () => { + const externalStore = createExternalStore({ + foo: 'bar', + gm: 'wagmi', + isGonnaMakeIt: false, + }) - let rerenders = -1 + let childRenderCount = 0 + const MemoComponent = React.memo((props: { store: any }) => { + childRenderCount++ + return
{props.store.isGonnaMakeIt}
+ }) - const TRIGGER_BTN_TEXT = 'Trigger re-render' + const renders: any[] = [] - const MemoComponent = memo((props: { store: any }) => { - rerenders++ - return
{props.store.isGonnaMakeIt}
+ function Test() { + const store = useExternalStore(externalStore, (state) => { + renders.push(state) }) + const [, rerender] = React.useState(0) + + return ( + <> + + + + ) + } - const Test = () => { - const store = useExternalStore(externalStore, () => {}) - const [, forceTestRerender] = useState(0) - - return ( - <> - - - - ) - } - - render() - const forceRerenderBtn = screen.getByText(TRIGGER_BTN_TEXT) - - expect(rerenders).toBe(0) - - fireEvent.click(forceRerenderBtn) // updating other state won't make the store be a new object + render() - expect(rerenders).toBe(0) + const forceRerenderBtn = screen.getByRole('button') + expect(childRenderCount).toBe(1) + expect(renders.length).toBe(1) - act(() => { - externalStore.set((x) => ({ ...x, isGonnaMakeIt: true })) // trigger rerendering when the used value changes - }) + // updating parent state, child should not rerender + fireEvent.click(forceRerenderBtn) + expect(childRenderCount).toBe(1) + expect(renders.length).toBe(2) - expect(rerenders).toBe(1) + // child and parent both rerender when store changes + act(() => { + externalStore.set((x) => ({ ...x, isGonnaMakeIt: true })) }) + expect(childRenderCount).toBe(2) + expect(renders.length).toBe(3) }) From 47f9c832453ead818d337a6e61b9c50ea9c1dba4 Mon Sep 17 00:00:00 2001 From: Tom Meagher Date: Fri, 27 Sep 2024 11:02:10 -0400 Subject: [PATCH 4/4] chore: changeset --- .changeset/neat-turkeys-camp.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/neat-turkeys-camp.md diff --git a/.changeset/neat-turkeys-camp.md b/.changeset/neat-turkeys-camp.md new file mode 100644 index 0000000000..e940e06b04 --- /dev/null +++ b/.changeset/neat-turkeys-camp.md @@ -0,0 +1,5 @@ +--- +"wagmi": patch +--- + +Stabilized `useAccount` return type object reference.