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

Infinite loop when using Suspense and key with multiple arguments, useMemo and useRef don't help #345

Closed
vimcaw opened this issue Apr 14, 2020 · 12 comments

Comments

@vimcaw
Copy link

vimcaw commented Apr 14, 2020

A simplified reproduction: https://codesandbox.io/s/useswr-bug-muyru?file=/src/App.js

When click the button, look the console panel, you can see many "recompute params".

image

import React, { useState, useMemo } from "react";
import { Suspense } from "react";
import useSWR, { SWRConfig } from "swr";
import ErrorBoundary from "./ErrorBoundary";

function Profile({ url }) {
  const params = useMemo(() => {
    console.count("recompute params");
    return { a: 1, b: 2 };
  }, []);
  const { data } = useSWR([url, params]);
  return <div>Content: {data}</div>;
}

export default function App() {
  const [url, setUrl] = useState();
  return (
    <SWRConfig
      value={{
        suspense: true,
        revalidateOnFocus: false,
        fetcher: (...args) => fetch(...args).then(res => res.json())
      }}
    >
      <ErrorBoundary>
        <Suspense fallback={<div>loading...</div>}>
          {url && <Profile url={url} />}
          <button onClick={() => setUrl("https://google.com/")}>
            Send a request
          </button>
        </Suspense>
      </ErrorBoundary>
    </SWRConfig>
  );
}
@sergiodxa
Copy link
Contributor

I think using objects inside the key array is not supported, except you create the object only one time, outside the component. This is because useMemo doesn't really guarantee your memoized object will be created only one time (React may ditch the memoized value at anytime)

@vimcaw
Copy link
Author

vimcaw commented Apr 15, 2020

Thank you for letting me know this, 😀 I just learned.

One of the solutions is using useMemoOne.

Can swr provide an option to custom diff function for the key? I want to do a deep-diff, although it is known that performance will be very poor, but in most instances, the params is very simple, such as { keyword: 'shoe', page: 1 }, I don't think this will have a big impact on performance.

As React official document says:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

source: https://reactjs.org/docs/hooks-reference.html#usememo

There are some discussions:
pbeshai/use-query-params#20
facebook/react#15278

@vimcaw
Copy link
Author

vimcaw commented Apr 15, 2020

After my multiple attempts, I found this problem don't caused by that React ditches the memoized value.

Create params using useRef:

https://codesandbox.io/s/useswr-suspense-bug-with-useref-bdmnr

function Profile({ url }) {
  const params = useRef({ a: 1, b: 2 }).current;
  console.count("render");
  const { data } = useSWR([url, params]);
  return <div>Content: {data}</div>;
}

This still causes infinite loop:

image

Create params outside component:

https://codesandbox.io/s/useswr-suspense-bug-with-params-out-of-component-jt5s9?file=/src/App.js

const params = { a: 1, b: 2 };
function Profile({ url }) {
  console.count("render");
  const { data } = useSWR([url, params]);
  return <div>Content: {data}</div>;
}

This don't causes infinite loop:

image

Conclusion

I Think this is because Suspense will unmount and remount the children when its state changes between pending and resolved, then creates a new params object every time when fetching.

This will cause inifite loop like:

  • useSWR fetching ->
  • Suspense unmount children ->
  • useSWR fetch done ->
  • Suspense remount children ->
  • recreate params object ->
  • key change, useSWR fetching ->
  • Suspense unmount children ->
    ...
    ...
    ...

Relative issue:

facebook/react#15921
facebook/react#14536

@vimcaw vimcaw changed the title Endless loop when using Suspense, multiple arguments and useMemo Infinite loop when using Suspense and multiple arguments, useMemo and useRef can't sovle it Apr 15, 2020
@vimcaw vimcaw changed the title Infinite loop when using Suspense and multiple arguments, useMemo and useRef can't sovle it Infinite loop when using Suspense and key with multiple arguments, useMemo and useRef don't help Apr 15, 2020
@vimcaw
Copy link
Author

vimcaw commented Apr 15, 2020

Solution

For static params

For static params, create it outside component:

const params = { a: 1, b: 2 };
function Profile({ url }) {
  console.count("render");
  const { data } = useSWR([url, params]);
  return <div>Content: {data}</div>;
}

For dynamic params(depends on other data)

1. Lifting params up to the components which are outside the Suspense component

https://codesandbox.io/s/useswr-suspense-bug-solution-i7j4b?file=/src/App.js

import React, { Suspense, useState, useMemo, useEffect } from "react";
import useSWR, { SWRConfig } from "swr";
import axios from "axios";
import ErrorBoundary from "./ErrorBoundary";

function Profile({ url, params }) {
  console.count("render");
  const { data } = useSWR([url, params]);
  return <div>Content: {data}</div>;
}

export default function App() {
  const [url, setUrl] = useState();
  const [varC, setVarC] = useState(3);

  const params = useMemo(
    () => ({
      pureUrl: url && url.replace(/^https?:\/\/(.+)$/, ""),
      b: 2,
      c: varC
    }),
    [url, varC]
  );

  useEffect(() => {
    if (url) {
      const timer = setTimeout(() => setVarC(6), 3000);
      return () => clearTimeout(timer);
    }
  }, [url]);

  return (
    <SWRConfig
      value={{
        suspense: true,
        revalidateOnFocus: false,
        fetcher: (url, params) => axios.get(url, { params })
      }}
    >
      <ErrorBoundary>
        <Suspense fallback={<div>loading...</div>}>
          {url && <Profile url={url} params={params} />}
          <button onClick={() => setUrl("https://google.com/")}>
            Send a request
          </button>
        </Suspense>
      </ErrorBoundary>
    </SWRConfig>
  );
}

2. Using a global memoize tool

To keep the reference equality, you can use a global memoize tool to memoize the params object.

There are some memoize tool:

Universal solution: manually serialize

First, create a helper function to serialize the params, for example:

import qs from 'qs';

export default function serialize(url: string, params?: object | any[]): string {
  if (typeof params === 'object' || Array.isArray(params)) {
    const matches = url.match(/^(.+?)(\?(.*))?$/);
    const urlWithoutQueryString: string = (matches && matches[1]) || url;
    const queryStringDataFromUrl: object = matches && matches[3] ? qs.parse(matches[3]) : {};
    const queryString: string = qs.stringify(
      { ...queryStringDataFromUrl, ...params },
      { arrayFormat: 'indices' }
    );
    if (queryString) {
      return `${urlWithoutQueryString}?${queryString}`;
    }
    return url;
  }
  return url;
}

Then, import the helper function to serialize the params, for the same URL and params, it always returns the same string.

import serialize from '../utils/serialize';

function Test({ filter }) {
  const { data } = useSWR(serialize('/goods', { keyword: 'shoe', page: 1 }));
}

@vimcaw
Copy link
Author

vimcaw commented Apr 15, 2020

So request again, can swr provide an option to custom diff function for the key? I want to do a deep-diff, although it is known that performance will be very poor, but in most instances, the params is very simple, such as { keyword: 'shoe', page: 1 }, I don't think this will have a big impact on performance.

And it can solve this problem, don't cause inifite loop like:

  • useSWR fetching ->
  • Suspense unmount children ->
  • useSWR fetch done ->
  • Suspense remount children ->
  • recreate params object ->
  • key change, useSWR fetching ->
  • Suspense unmount children ->
    ...
    ...
    ...

@shuding
Copy link
Member

shuding commented Apr 15, 2020

Amazing investigation!

I Think this is because Suspense will unmount and remount the children when its state changes between pending and resolved, then creates a new params object every time when fetching.

Yeah I think states from useMemo or useRef will be ditched when it throws inside Suspense and enters again, for correctness. This will be very tricky for us.

I think generally we have 2 solutions:

Serialization

I’d go this way:

useSWR(JSON.stringify(params), () => fetcher(params))

Of course you can use any serialization lib here. And here we don’t see key as params of the fetcher, it’s just a “key” of the request. Instead it just tells the hook to refetch when params has changed, and return the same data if it’s cached.

Global Memorization

You can use global memorization as well, but I won’t recommend this way because it involves an extra mental layer and will possibly cause a memory leaking.

@vimcaw
Copy link
Author

vimcaw commented Apr 17, 2020

Thanks! Serialization is a good way to solve it, but it still make some boilerplate code, and in many case, we will encapsulates fetcher outside, so we need to import files every time, we hope to configure it only once in SWRConfig.

import customFetcher from '@/utils/customFetcher'

function Test({ filter }) {
  const params = useMemo(() => ({ ...filter, page: 1, pageSize: 10 }), [filter]);
  useSWR(JSON.stringify(params), () => customFetcher('goods', params))
}

I understand this is a trade-off, also agree that global memory is very bad, there are no perfect way.

I think it's better if swr provide an option to custom diff function for the key, in most instance, the performance of deepDiff(params) and JSON.stringify(params) will not be too far apart.

This can simplify the usage to:

function Test({ filter }) {
  useSWR(['goods', { ...filter, page: 1, pageSize: 10 }])
}

export defult App() {}

And just configure customFetcher only once in SWRConfig.

import customFetcher from "@/utils/customFetcher";
import deepDiff from "deep-diff";

<SWRConfig
  value={{
    suspense: true,
    keyDiffer: (prevKey, currentKey) =>
      typeof prevKey === "string" && typeof currentKey === "string"
        ? prevKey === currentKey
        : deepDiff(prevKey, currentKey),
    fetcher: (...args) => customFetcher(...args)
  }}
>
  <ErrorBoundary>
    <Suspense fallback={<div>loading...</div>}>
      <Test filter={{ keyword: "shoe" }} />
    </Suspense>
  </ErrorBoundary>
</SWRConfig>;

Of course it's better if React can come up with a solution. I will ask this question in React's issue when I have time.

@ddhp
Copy link

ddhp commented Apr 18, 2020

Isn't this the same scenario explained in Multiple Arguments? Although in this case they would pass shallow comparing.

From my understanding(please correct me if I am wrong), the reason they are treated as different keys is because in the hash module, WeakMap is utilized so the key keeps changing then causing revalidating. I am wondering what's the reason behind it to not JSON.stringify it like treating other types(null, undefined, number)?

@vimcaw
Copy link
Author

vimcaw commented Jul 9, 2020

Share my current solution:

First, create a helper function to serialize the params, for example:

import qs from 'qs';

export default function serialize(url: string, params?: object | any[]): string {
  if (typeof params === 'object' || Array.isArray(params)) {
    const matches = url.match(/^(.+?)(\?(.*))?$/);
    const urlWithoutQueryString: string = (matches && matches[1]) || url;
    const queryStringDataFromUrl: object = matches && matches[3] ? qs.parse(matches[3]) : {};
    const queryString: string = qs.stringify(
      { ...queryStringDataFromUrl, ...params },
      { arrayFormat: 'indices' }
    );
    if (queryString) {
      return `${urlWithoutQueryString}?${queryString}`;
    }
    return url;
  }
  return url;
}

Then, import the helper function to serialize the params, for the same url and params, it always returns the same string.

import serialize from '../utils/serialize';

function Test({ filter }) {
  const { data } = useSWR(serialize('/goods', { keyword: 'shoe', page: 1 }));
}

@shuding
Copy link
Member

shuding commented Dec 24, 2021

Since SWR 1.1.0, there is built-in key serialization so you don’t have to handle this manually! 🎉 Check the release note here: https://github.com/vercel/swr/releases/tag/1.1.0

@shuding shuding closed this as completed Dec 24, 2021
@adnanfuat
Copy link

adnanfuat commented Dec 30, 2021

I think using objects inside the key array is not supported, except you create the object only one time, outside the component. This is because useMemo doesn't really guarantee your memoized object will be created only one time (React may ditch the memoized value at anytime)

6 hours gone. Until I see your message. You saved my day. Thanks @sergiodxa

@shuding
Copy link
Member

shuding commented Dec 30, 2021

@adnanfuat That is not true with the latest version, please try upgrading.

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

5 participants