Skip to content

refactor: use getter functions to avoid unnecessary re-renders #890

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 12, 2021
Merged

refactor: use getter functions to avoid unnecessary re-renders #890

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2fa533f
refactor: use Object.assign to avoid unnecessary re-renders
koba04 Jan 11, 2021
5cf5683
refactor: use getter functions to avoid unnecessary re-renders
koba04 Jan 11, 2021
116fced
refactor: use Object.assign to avoid unnecessary re-renders
koba04 Jan 12, 2021
ac512da
Revert "refactor: use Object.assign to avoid unnecessary re-renders"
koba04 Jan 12, 2021
352e03f
refactor: define properties with enumerable: true
koba04 Jan 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/use-swr-infinite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,12 +216,12 @@ function useSWRInfinite<Data = any, Error = any>(
[mutate, pageCountCacheKey]
)

return {
...swr,
// Use Object.assign to avoid unnecessary re-renders caused by triggering all the getters of the returned swr object
return Object.assign({}, swr, {
Copy link
Member

@huozhi huozhi Jan 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Object.assign seems to have the same effect with object spread?

var a = { get x() { console.log('getx'); return 'x' } }
Object.assign({}, a) // "getx"

I think you can just assign to swr itself? the 1st arg won't be traversed.

Suggested change
return Object.assign({}, swr, {
return Object.assign(swr, {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback!
You are right, to avoid triggering the getter function, I have to mutate a directly.

var a = {
  get x() { console.log('getx'); return 'x' }
}

Object.assign(a, { foo: 'foo'}) // dosn't trigger the getter

But, that would break the current behavior, so I'll investigate a way to avoid triggering re-renders.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A way I came up with is to define getter functions to evaluate properties lazily.

diff --git a/src/use-swr-infinite.ts b/src/use-swr-infinite.ts
index 3b26b67..949cdc8 100644
--- a/src/use-swr-infinite.ts
+++ b/src/use-swr-infinite.ts
@@ -216,12 +216,16 @@ function useSWRInfinite<Data = any, Error = any>(
     [mutate, pageCountCacheKey]
   )
 
-  // Use Object.assign to avoid unnecessary re-renders caused by triggering all the getters of the returned swr object
-  return Object.assign({}, swr, {
+  // Use getter functions to avoid unnecessary re-renders caused by triggering all the getters of the returned swr object
+  return {
+    get error() { return swr.error },
+    get data() { return swr.data },
+    get revalidate() { return swr.revalidate },
+    get isValidating() { return swr.isValidating },
     mutate,
     size,
     setSize
-  }) as SWRInfiniteResponseInterface<Data, Error>
+  } as SWRInfiniteResponseInterface<Data, Error>
 }
 
 export {

Copy link
Member

@shuding shuding Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain a bit why Object.assign(swr, { ... }) won't work? It does trigger the getter when accessing a.x:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shuding
The reason is that this mutates swr directly, and it breaks where the swr.mutate variable is used.

swr/src/use-swr-infinite.ts

Line 198 in 3121592

return swr.mutate(data, shouldRevalidate)

It leads to failing many tests. I can fix it like the following, but the useSWRInfinite › should keep mutate referential equal test is still failing. Should I investigate the way to pass the test rather than using getter functions?

diff --git a/src/use-swr-infinite.ts b/src/use-swr-infinite.ts
index a0eda9c..ab0541e 100644
--- a/src/use-swr-infinite.ts
+++ b/src/use-swr-infinite.ts
@@ -184,6 +184,8 @@ function useSWRInfinite<Data = any, Error = any>(
     dataRef.current = swr.data
   }, [swr.data])

+  const _mutate = swr.mutate
+
   const mutate = useCallback(
     (data, shouldRevalidate = true) => {
       if (shouldRevalidate && typeof data !== 'undefined') {
@@ -195,9 +197,9 @@ function useSWRInfinite<Data = any, Error = any>(
         cache.set(contextCacheKey, { force: true })
       }

-      return swr.mutate(data, shouldRevalidate)
+      return _mutate(data, shouldRevalidate)
     },
-    [swr.mutate, contextCacheKey]
+    [_mutate, contextCacheKey]
   )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failed test is here.

swr/test/use-swr-infinite.test.tsx

Line 441 in 3121592

expect(setSize).toEqual(setters[0])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point! I think we need to find an elegant way to fix it then... let me see

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shuding I agree with you, that's not an ideal solution. I'll try to find it. Thank you for your feedback!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the same logic like use-swr then:

const swrInfinite = { size, setSize, mutate }
Object.defineProperties(swrInfinite, {
  error: {
    get: () => swr.error,
    enumerable: true
  },
  ...
})

The reason of not using the latest get syntax is that we have to specify enumerable: true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated specifing enumerable: true!

I also came up with the way to use useMemo like 116fced, but React says "You may rely on useMemo as a performance optimization, not as a semantic guarantee. ", so I've reverted the change.

mutate,
size,
setSize
} as SWRInfiniteResponseInterface<Data, Error>
}) as SWRInfiniteResponseInterface<Data, Error>
}

export {
Expand Down