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

vimcaw commented 4 years ago

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".

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 commented 4 years ago

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 commented 4 years ago

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: https://github.com/pbeshai/use-query-params/issues/20 https://github.com/facebook/react/issues/15278

vimcaw commented 4 years ago

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:

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:

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:

https://github.com/facebook/react/issues/15921 https://github.com/facebook/react/issues/14536

vimcaw commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 2 years ago

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

adnanfuat commented 2 years ago

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 commented 2 years ago

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

vercel / swr