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

feat(nuxt): server-only pages #24954

Merged
merged 51 commits into from Feb 26, 2024
Merged

feat(nuxt): server-only pages #24954

merged 51 commits into from Feb 26, 2024

Conversation

huang-julien
Copy link
Member

@huang-julien huang-julien commented Dec 29, 2023

πŸ”— Linked issue

❓ Type of change

#19772

  • πŸ“– 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 allows users to define pages as server page with definePageMeta() and the .server.vue suffix in the filename.

in definePageMeta this PR took #25037 as reference for the field name : mode. For now it only accept undefined or 'server'. For the client only implementation, see #25037

πŸ“ Checklist

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

Copy link

stackblitz bot commented Dec 29, 2023

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

@huang-julien huang-julien changed the title feat(nuxt): define island pages with definePageMeta macro feat(nuxt): server only pages Feb 11, 2024
Co-authored-by: Daniel Roe <daniel@roe.dev>
@@ -160,6 +162,7 @@ export default defineComponent({
// TODO: Validate response
// $fetch handles the app.baseURL in dev
const r = await eventFetch(withQuery(((import.meta.dev && import.meta.client) || props.source) ? url : joinURL(config.app.baseURL ?? '', url), {
url: route.fullPath,
Copy link
Member

@danielroe danielroe Feb 25, 2024

Choose a reason for hiding this comment

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

I think this will make it much harder to cache server islands as the hash we create will vary by path.

Could we instead pass this as part of the context via createIslandPage instead (and maybe use route.path to skip query/hash)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should not skip query πŸ€”, userss tend to relly on it within pages composables. Maybe can we pass an object like

{
  path: string,
query?: Record<string, string>
}

?

Copy link
Member

Choose a reason for hiding this comment

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

The issue is (for example) that we will make a separate request for every query variation, which wouldn't be compatible with static sites and allow cache-busting by passing arbitrary query params or hashes ... (We should definitely not pass hash.) We could document this as a limitation of server pages... Or at a minimum, skip passing the full route if the route matches prerender routeRule?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, let's start by passing only the path in the context then πŸ€”. and iterate on it, i'm not sure what kind of side effect we can have if using fullPath with ssg

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

this looks great to me.

I do think we probably could respect the route query as long as it's not a prerendered page. (We can use isPrerendered to tell us.).

And I think we should drop the support for defining server-only pages within definePageMeta, which means that it's impossible to tree-shake this code out of the bundle, if even one page has any metadata at all.

packages/nuxt/src/pages/utils.ts Fixed Show fixed Hide fixed
packages/nuxt/src/pages/utils.ts Fixed Show fixed Hide fixed
packages/nuxt/src/pages/utils.ts Dismissed Show dismissed Hide dismissed
@danielroe danielroe changed the title feat(nuxt): server only pages feat(nuxt): server-only pages Feb 26, 2024
@danielroe danielroe merged commit 196223c into main Feb 26, 2024
36 of 37 checks passed
@danielroe danielroe deleted the feat/server-page branch February 26, 2024 17:39
@github-actions github-actions bot mentioned this pull request Feb 26, 2024
@brc-dd
Copy link

brc-dd commented Feb 29, 2024

There seems to be an issue with this if you try to directly access the server-only page in dev:

[nitro] [unhandledRejection] FetchError: [GET] "/_nuxt/builds/meta/dev.json": 404 Page not found: /_nuxt/builds/meta/dev.json
    at async $fetch2 (file:///Users/brc-dd/nuxt-app/node_modules/.pnpm/ofetch@1.3.3/node_modules/ofetch/dist/shared/ofetch.00501375.mjs:261:15)
[Vue warn]: Component foo is missing template or render function.

It works fine if you open home first and then go to server-only page via link πŸ‘€

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

3 participants