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): add baseURL to island fetch requests #22009

Merged
merged 10 commits into from Jul 18, 2023

Conversation

huang-julien
Copy link
Member

@huang-julien huang-julien commented Jul 7, 2023

πŸ”— Linked issue

resolve #22002
resolve #22010

❓ 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 πŸ‘‹ this PR prepend the fetch request of NuxtIsland with the baseURL or it wouldn't correctly hit the endpoint of islands renderer

πŸ“ Checklist

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

@danielroe can i set a app.baseURL for fixtures/basic ?

todo

  • non-reg test with NUXT_APP_BASE_URL within fixtures tests

@stackblitz
Copy link

stackblitz bot commented Jul 7, 2023

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

@huang-julien huang-julien self-assigned this Jul 7, 2023
@@ -79,7 +81,7 @@ export default defineComponent({
appendResponseHeader(event, 'x-nitro-prerender', url)
}
// TODO: Validate response
const r = await eventFetch(withQuery(url, {
const r = await eventFetch(withQuery(join(appConfig.app.baseURL ?? '', url), {
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this is only needed for prod+client, where we use globalThis.fetch?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm we can't really build the test/fixtures/basic directory due to #22010. But the issue is also happening in dev

Copy link
Member

@danielroe danielroe Jul 7, 2023

Choose a reason for hiding this comment

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

Ah, missed that.

We'll have to do this the other way then - omit the baseURL for dev+client (because we are using $fetch there which comes with it preconfigured).

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
Copy link
Member

danielroe commented Jul 7, 2023

Regarding setting app.baseURL we do this already dynamically in one of the tests via an environment variable - maybe you could reproduce the issue in a regression test that way?

@huang-julien
Copy link
Member Author

Sure ! I'll convert this PR to draft. Will finish in a few days

@huang-julien huang-julien marked this pull request as draft July 7, 2023 14:21
@danielroe danielroe marked this pull request as ready for review July 18, 2023 14:54
@danielroe danielroe changed the title fix(nuxt): add baseURL to NuxtIsland fetch request fix(nuxt): add baseURL to island fetch requests Jul 18, 2023
@danielroe danielroe merged commit f4ec04f into nuxt:main Jul 18, 2023
28 checks passed
This was referenced Jul 18, 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.

Prerendering is not working with app.baseURL Server components break during SSR if you set a baseURL
2 participants