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): use $fetch.raw in dev client mode for islands #21904

Merged
merged 3 commits into from Jul 5, 2023

Conversation

huang-julien
Copy link
Member

πŸ”— Linked issue

fix #21740

❓ 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

Hi πŸ‘‹

There's currently a bug that can happen on client-side nav in dev mode (did not test build mode and don't know why this hasn't been detected by the fixtures tests).

bug reproduction:

  • run test/fixtures/basic in dev
  • go to localhost:3000
  • click on the islands ``NuxtLink

Sometimes when navigating to a page containing a server component/island, we can have a 503 response by the server. With the native fetch API, this response isn't handled and breaks completly the page.

reproduction env


  • Operating System: Windows_NT
  • Node Version: v18.16.1
  • Nuxt Version: 3.6.1
  • Nitro Version: 2.5.2
  • Package Manager: pnpm@7.9.0
  • Builder: vite
  • User Config: -
  • Runtime Modules: -
  • Build Modules: -

πŸ“ Checklist

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

@stackblitz
Copy link

stackblitz bot commented Jul 3, 2023

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

@@ -81,7 +82,7 @@ export default defineComponent({
...props.context,
props: props.props ? JSON.stringify(props.props) : undefined
}))
const result = await r.json() as NuxtIslandResponse
const result = process.server ? await r.json() : (r as FetchResponse<NuxtIslandResponse>)._data
Copy link
Member

Choose a reason for hiding this comment

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

can we resolve this issue by just adding a .catch block to r.json()?

I also can't reproduce the issue on my end so my guess is that it's an issue with nitro's dev server socket + windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes i already suspected that this was a windows issue. $fetch was solving this by rerunning the request.

Catching the .json ? Can't we check the response status (503) and refetch just like $fetch ?

Should we just allow an infinite refetch on 503 instead of a single catch ? It's quite rare but i can have up to 3 failed 503 request.

Copy link
Member

Choose a reason for hiding this comment

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

Hm. It feels like this is an issue that needs to be resolved in Nitro rather than worked around in this kind of way. That said, I'm happy to merge this as a hotfix if essential.

Have you tried with the latest nitropack version? There was a fix merged that should have improved things on Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

you're probably referring to unjs/nitro#1115 πŸ€” ? on the edge version, i still have the 503 issue (nitropack 2.5.2)

would love to have someone on windows to also confirm this

Copy link
Member

Choose a reason for hiding this comment

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

yes, exactly. any chance you could see if you can provide a pure nitro reproduction for @pi0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure πŸ‘

@danielroe danielroe changed the title fix(nuxt): use $fetch.raw client-side for NuxtIsland fix(nuxt): use $fetch.raw in dev client mode for islands Jul 5, 2023
@danielroe danielroe merged commit 52a427d into nuxt:main Jul 5, 2023
28 checks passed
@github-actions github-actions bot mentioned this pull request Jul 5, 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.

None yet

2 participants