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: preview pre-rendered pages #14836

Closed

Conversation

brillout
Copy link
Contributor

Description

The HTML of pre-rendered pages lives in dist/, therefore it makes sense to serve .html files also for appType: 'custom'.

Additional context

This fixes a regression introduced by #14756.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Oct 31, 2023

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

@brillout
Copy link
Contributor Author

Someone should run /ecosystem-ci run against this PR.

@bluwy
Copy link
Member

bluwy commented Nov 1, 2023

I'm not sure if this is a bug? Like you mentioned, it's using appType: 'custom' so you have to handle HTML requests like in dev too, so I think it's better aligned now. The appType config is top-level instead of nested to server.*.

This should also be covered in the migration guide though which I missed.

EDIT: Unless we say appType: 'custom' has no effect in preview, since it's not common for that in preview especially without middlewareMode there. But it would still be nice to align with dev if there's ever a usecase for custom.

@brillout
Copy link
Contributor Author

brillout commented Nov 1, 2023

In dev, a page is always SSR'd, regardless of whether the user chooses to pre-render that page.

In preview, when a page is pre-rendered, the page isn't SSR'd and, instead, its HTML is generated at build time and lives in dist/ as a static .html file, for example dist/hello/index.html or dist/hello.html (both ${url}.html and ${url}/index.html are common as some static hosts prefer the one over the other).

From that perspective, the discrepancy between dev and preview is very much expected.

From a technical point of view, I understand this adds an edge case, but I'd argue it's the correct way to do it.

@bluwy
Copy link
Member

bluwy commented Nov 1, 2023

Isn't that Vike specific? In build/preview, you can also choose not to prerender some pages, and handle SSR for the requested pages. Or a hybrid mode where some HTML is prerendered, and some not that goes through SSR. IIUC the discrepancy isn't within Vite.

@brillout
Copy link
Contributor Author

brillout commented Nov 1, 2023

In build/preview, you can also choose not to prerender some pages, and handle SSR for the requested pages. Or a hybrid mode where some HTML is prerendered, and some not that goes through SSR.

Yes, exactly, and that's precisely why both is needed: the SSR middleware + serving static .html files.

Isn't that Vike specific? [...] IIUC the discrepancy isn't within Vite.

If Vite's preview doesn't serve .html files, then Vike will have to implement/duplicate the whole .html serving logic.

The purpose of Vite's preview server is to alleviate users and framework authors from implementing their own preview server and re-inventing the wheel.

It's specific to Vike and any (future) framework that (will) want to support partial pre-rendering.

This isn't a blocker for Vike and I don't mind re-implementing the .html serving logic in Vike, I just think that this PR is correct way for Vite to do it.

@bluwy
Copy link
Member

bluwy commented Nov 1, 2023

We also talked about this in the meeting and prefer aligning in dev at the meantime. If appType: 'custom' is specified, from a plugin pov if it handles middlewares in dev, it would also do the same in preview.

It also seems like the behaviour you want is the same as appType: 'mpa', however presumably you'd need to know where you're in dev or preview to set it differently, which leads to #12298 (which we also discussed and will comment later).

So since users can specify appType: 'mpa' if they want this today, we don't think this is needed.

@brillout
Copy link
Contributor Author

brillout commented Nov 2, 2023

It also seems like the behaviour you want is the same as appType: 'mpa'

👍 Makes sense.

I tried to make it work with appType: 'mpa' but couldn't. So I've decided to fallback to what other frameworks do: always use the SSR middleware even for pre-rendered pages. It's a pity because it isn't a faithful preview anymore, but I guess this "cheat" is ok.

Anyways, the decision to reject this PR makes sense, thus closing.

@brillout brillout closed this Nov 2, 2023
@brillout brillout deleted the brillout/fix-align-html-serving branch September 18, 2024 08:14
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.

2 participants