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

[BUG] useSelect hook ignores staleTime #5843

Closed
misterx opened this issue Apr 12, 2024 · 8 comments · Fixed by #5851 or #5910
Closed

[BUG] useSelect hook ignores staleTime #5843

misterx opened this issue Apr 12, 2024 · 8 comments · Fixed by #5851 or #5910
Labels
bug Something isn't working
Milestone

Comments

@misterx
Copy link

misterx commented Apr 12, 2024

Describe the bug

I'm facing the same problem described in #1815, where useSelect duplicates requests for Options list, but in this case it ignores staleTime option.

Steps To Reproduce

  1. Go to https://codesandbox.io/p/devbox/charming-bohr-jt6tcl?file=%2Fsrc%2Fcomponents%2FResourceSelect.tsx%3A10%2C37
  2. Edit post
  3. Check the browser console's requests tab.

Expected behavior

The options list should be loaded only once.

Packages

"@refinedev/cli": "^2.16.21",
"@refinedev/core": "^4.47.1",
"@refinedev/devtools": "^1.1.32",
"@refinedev/kbar": "^1.3.6",
"@refinedev/simple-rest": "^5.0.1",
"@refinedev/antd": "^5.37.4",
"antd": "^5.0.5",

Additional Context

I've created another simple component, ResourceUseQuerySelect, that uses just the useQuery hook and makes requests via datasource.getList. It seems to have the correct behavior for requesting data.
Both components are used on the same page and form.

Upd: I've tried to call useList hook in the component, and it duplicates requests too. It looks like there might be something wrong with the useList hook because, under the hood, it uses useQuery, which works fine.

I also tested in prod mode, and non strict mode.

@misterx misterx added the bug Something isn't working label Apr 12, 2024
@misterx
Copy link
Author

misterx commented Apr 12, 2024

I found why the duplication happens. I copied the useList hook into my local project for debugging and discovered that accessing the signal from queryFn triggers refetching

queryFn: ({ queryKey, pageParam, signal }) => {

When I removed the signal from queryFn, everything worked well. I also found a related ticket about this issue: TanStack/query#6089.

I'm not sure how I can fix this from my project.

@aliemir
Copy link
Member

aliemir commented Apr 15, 2024

Hey @misterx, sorry for the issue! I've tested in my local using the code you've provided but haven't see anything unusual 🤔 I've observed that the ResourceSelect ends up sending a duplicate request due to strict mode. When I disable strict mode or test it in production I can see that there's no duplicate request. Do you have a duplicate request in production or when you disable strict mode?

About the signal issue you've referenced. I think we can fix this, we can refactor those lines to not use destructuring and make access of signal property with a getter to avoid unexpected side effects from @tanstack/react-query. I think doing so will also prevent duplicate requests (as you've experimented in your local) in strict mode.

@misterx
Copy link
Author

misterx commented Apr 15, 2024

My investigation has revealed the following issues:

Duplicate Requests:
Each input component is mounted three times, causing three duplicate requests:

Component Remounting:
According to the antd documentation, using resetFields leads to the remounting of input components (see more at https://ant.design/components/form#why-will-resetfields-re-mount-component).

Caching and Query Cancelation:
Every new render triggers a new request because the useList hook consumes the signal property in queryFn.
The useQuery documentation (https://tanstack.com/query/v4/docs/framework/react/guides/query-cancellation#default-behavior) states that caching will occur on remounts if the signal property is not consumed. If it is, then the responsibility for cancelling the request falls to the client code.

Behavior Control in Older Versions:
In earlier versions of Refine where the signal property is not utilized, client-side behavior can be managed using the staleTime property in queryOptions.

@aliemir I've created sandbox https://codesandbox.io/p/devbox/pedantic-albattani-cymd3r where I removed strict mode and see three identical requests.

image

I also created a simple component that shows the input remounting.

export const TestInput: React.FC = () => {
  const ref = useRef(0);
  ref.current++;
  console.log("Current ref");
  console.log(ref.current);
  return <></>;
};

Result in the console (you can see the same in the sandbox):
image

As you can see, the component is created three times.

@misterx
Copy link
Author

misterx commented Apr 15, 2024

Also, one unnecessary render can be removed if resetFields() is called when queryResult?.data?.data is not undefined.

https://github.com/refinedev/refine/blob/master/packages/antd/src/hooks/form/useForm.ts#L262-L264

@aliemir
Copy link
Member

aliemir commented Apr 15, 2024

Thank you for the investigation @misterx! #5831 #5851 will prevent the consumption of signal and restore the older behavior for your case then. About the useEffect calling resetFields(), I think it makes sense, if you want to open up a PR for this change, we'll be happy to include it in our next release 🚀

Linked the wrong PR 🤦

@misterx
Copy link
Author

misterx commented Apr 16, 2024

@aliemir I would be glad to contribute to the project, but I'm not sure I have enough knowledge to commit since I'm a backend developer and not familiar with frontend development tools such as testing and building tools. After my research, I understand why Refine uses form.resetFields(): it's necessary to set the initial values from the request. I've researched how others address this issue to avoid re-rendering and found a solution from AntProForm

https://github.com/ant-design/pro-components/blob/9daed82b9a1617131cfd22393969131e1f45568c/packages/form/src/BaseForm/BaseForm.tsx#L720-L729

they also load initial values from a request but do not render the form until the values are ready. Currently, I can't use this approach with the existing Refine useForm because the form.resetFields call from refine.useForm is not customizable and can't be disabled.

React.useEffect(() => {
form.resetFields();
}, [queryResult?.data?.data, id]);

So, I've created a simple hack (I'm not sure this is a good solution, but it works) that prevents unnecessary form refreshes on the Edit form until the data is defined.

const useFixFormResetOnLoad:(form:FormInstance,initialValues?:any)=>void = (form,initialValues) => {
    const originalReset = useRef<FormInstance['resetFields']>(form.resetFields);
    useEffect(()=>{
       form.resetFields = initialValues===undefined?()=>undefined:originalReset.current;
    },[initialValues, form])
}

And edit page will be something like:

export const BlogPostEdit = () => {

  const {form,formProps,saveButtonProps, queryResult, formLoading } = useForm();
  useFixFormResetOnLoad(form,queryResult?.data?.data);
  if(formLoading){
    return (
        <div style={{ paddingTop: 50, paddingBottom: 50, textAlign: 'center' }}>
	    <Spin />
        </div>
    );
  }

   return (
    <Edit saveButtonProps={saveButtonProps}>
        <Form layout="vertical" {...formProps}>
        {/*.............................*/}
        {/*.... standard refine code ...*/}
        {/*.............................*/}
        </Form>
    </Edit>
}

This behavior is good for me because it avoids unnecessary re-rendering for complex forms. However, I'm not sure if this approach will be suitable for others, as form is not visible durring initialData loading.

@aliemir
Copy link
Member

aliemir commented Apr 16, 2024

@misterx thank you for taking your time for the investigation and explanation. Understood the issue here and your workaround. If I understood correctly, we can try to add a simple condition here at

React.useEffect(() => {
form.resetFields();
}, [queryResult?.data?.data, id]);

and then the initial rendering can be handled by users by conditionally rendering via formLoading, am I correct?

We may also do some hacks in formProps (like the one you did in useFixFormResetOnLoad) to trick initial rendering but this will change the current behavior and might be unwanted in some cases.

In conclusion, we'll make form.resetFields to be called conditionally in the effect and leave the handling of the initial render to the user. 🙌 Until this is done and released, your workaround will work the way we expect 🚀

@misterx
Copy link
Author

misterx commented Apr 16, 2024

@aliemir Yes, you are correct. Also, since the first and second renders are the same (the state is unchanged and the initialValues are empty), form.resetFields can be called only once when the data is received. This will eliminate the unnecessary second re-render and should not impact existing client code.
Furthermore, removing re-rendering will enhance the framework's functionality, allowing for simpler use of Form.List with useSelect. An example use case is creating an invoice with lines where the product on each line should be a dropdown from the products resource (this is similar to the use case that prompted this investigation). I see the refine-crm example with a similar form on the QuotesShow page https://example.crm.refine.dev/quotes/show/84, but without dropdowns ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants