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

fix(nuxt): respect custom timeout in useFetch #24364

Merged
merged 11 commits into from Nov 20, 2023
Merged

Conversation

luc122c
Copy link
Contributor

@luc122c luc122c commented Nov 18, 2023

πŸ”— Linked issue

Fixes #24354

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Implemented timeout option in useFetch as outlined in @danielroe's comment

Tested using the provided reproduction in the playground:

Before:
Screenshot 2023-11-18 at 11 41 20 pm

After:
Screenshot 2023-11-18 at 11 41 37 pm

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

stackblitz bot commented Nov 18, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@luc122c luc122c marked this pull request as ready for review November 18, 2023 23:48
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests βœ…

❗ No coverage uploaded for pull request base (main@7cce0ef). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24364   +/-   ##
=======================================
  Coverage        ?   58.76%           
=======================================
  Files           ?        5           
  Lines           ?      861           
  Branches        ?       46           
=======================================
  Hits            ?      506           
  Misses          ?      355           
  Partials        ?        0           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

Copy link

nuxt-studio bot commented Nov 19, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
Nuxt Docs Edit on Studio β†—οΈŽ View Live Preview abc60d3

@luc122c
Copy link
Contributor Author

luc122c commented Nov 19, 2023

I've added a test to cover the change. I tried registering a new endpoint with a delay, however since internal requests don't make network requests, there was no effect. Therefore, this test does need to make a network request to an external URL. Is there a better one to use than this?

@manniL
Copy link
Member

manniL commented Nov 19, 2023

Is there a better one to use than this?

You could probably use useAsyncData + an own custom promise? πŸ€”

const { pending, status, error } = await useAsyncData(
  () => new Promise(resolve => setTimeout(resolve, 5000)), 
  { timeout: 1 }
)

@pi0
Copy link
Member

pi0 commented Nov 19, 2023

Thanks for this!

I made an upstream tracker (unjs/ofetch#326) would you please add a TODO comment above about the migration plan and why we need this workaround today? πŸ™πŸΌ

For ofetch, we are considering AbortSignal.timeout (unjs/ofetch#309). Nuxt can opt-in sooner for this while you are at it.

@luc122c
Copy link
Contributor Author

luc122c commented Nov 20, 2023

Thanks for your notes @manniL and @pi0. Please take a look at the latest changes and see if they're ok for you?

@danielroe danielroe requested a review from pi0 November 20, 2023 10:10
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Other than small comment LGTM thanks

@danielroe danielroe changed the title fix(nuxt): implement timeout in useFetch fix(nuxt): respect custom timeout in useFetch Nov 20, 2023
@danielroe danielroe merged commit 6e44b1b into nuxt:main Nov 20, 2023
35 checks passed
@danielroe
Copy link
Member

Thank you ❀️

@github-actions github-actions bot mentioned this pull request Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The timeout option of useFetch has no effect
6 participants