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

[RFC] 🥅 Improve feedback for infinite query loops #1531

Closed

Conversation

Shrugsy
Copy link
Collaborator

@Shrugsy Shrugsy commented Sep 20, 2021

  • Throw an error when query args are detected as changing every
    render too frequently, after factoring shallow stabilisation attempts
  • Expand docs explanation about stable query arg requirements

The topic of infinite re-renders with unstable query args comes up every now and then (e.g. #1526). This aims to provide more explicit, immediate feedback about the cause, and point to the relevant part of the docs as a solution.

A few things to consider:

  • Is this safe from false positives?

    I think it should be safe. The criteria to throw an error is "value changes 10 times in 10 consecutive renders within 1000ms, after attempting stabilization". I don't think that criteria would ever intersect with intentional behaviour.

  • Should it actually be throwing an error, or only logging an error message?

    I think throwing an error is better here. If it's meeting that criteria, I think it's a safe bet to assume that it will either freeze your browser tab, continue looping indefinitely, or throw react's internal infinite loop error ("Uncaught Error: Too many re-renders. React limits the number of renders to prevent an infinite loop."). I believe that throwing our error earlier is more useful than just logging alongside any of those three options, and has less chance of being missed.

  • Is the additional runtime code overhead actually worth having for what is a relatively niche case?

    tbh I'm not sure about this

@netlify
Copy link

netlify bot commented Sep 20, 2021

✔️ Deploy Preview for redux-starter-kit-docs ready!

🔨 Explore the source changes: 2539fff

🔍 Inspect the deploy log: https://app.netlify.com/sites/redux-starter-kit-docs/deploys/6148c6110108170007247d30

😎 Browse the preview: https://deploy-preview-1531--redux-starter-kit-docs.netlify.app

- Throw an error when query args are detected as changing every
  render too frequently, after factoring shallow stabilisation attempts
- Expand docs explanation about stable query arg requirements
@Shrugsy Shrugsy force-pushed the qol/improve-infinite-loop-feedback branch from bf7b45e to 2539fff Compare September 20, 2021 17:34
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2539fff:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration

@Shrugsy
Copy link
Collaborator Author

Shrugsy commented Sep 20, 2021

CSB examples are here to demo the new behaviour. For the setup: a component is added which uses a nested argument for a query which always returns an error. The component will be rendered once you click the 'Toggle error query' button.

v1.6.1 behaviour example

Note: The tab might freeze or you might observe it looping indefinitely in the console.
https://codesandbox.io/s/examples-query-react-basic-with-nested-arg-error-forked-08g8n?file=/src/App.tsx

Branch example

Rather than freezing or looping indefinitely, a meaningful error message is instead shown.
https://codesandbox.io/s/examples-query-react-basic-with-nested-arg-error-kxl7b?file=/src/App.tsx

@Shrugsy Shrugsy marked this pull request as ready for review September 20, 2021 18:05
@msutkowski msutkowski self-requested a review September 20, 2021 18:27
@Shrugsy
Copy link
Collaborator Author

Shrugsy commented Sep 21, 2021

Closed due to #1533

@Shrugsy Shrugsy closed this Sep 21, 2021
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

Successfully merging this pull request may close these issues.

1 participant