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(html)!: align html serving between dev and preview #14756

Merged
merged 7 commits into from
Oct 30, 2023
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 25, 2023

Description

Supersedes #11289

Fixes #6596
Fixes #14690

New behaviour:

Before (dev) Before (preview) After (dev & preview)
nested/index.html nested/index.html nested/index.html nested/index.html
nested index.html (mpa 404) nested/index.html index.html (mpa 404)
nested/ nested/index.html nested/index.html nested/index.html
non-nested.html non-nested.html non-nested.html non-nested.html
non-nested index.html (mpa 404) non-nested.html non-nested.html
non-nested/ index.html (mpa 404) non-nested.html index.html (mpa 404)

Additional context

HTML files now also don't have charset=utf-8 by default. I'm not really sure how to handle that and I've come to agree that it's also off that it's misaligned. Previously charset=utf-8 is automatically added by sirv by default in preview, but not in dev.

Also removed connect-history-api-fallback and implement the fallback ourselves.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Co-authored-by: 翠 / green <green@sapphi.red>
@bluwy bluwy added the p3-significant High priority enhancement (priority) label Oct 25, 2023
@stackblitz
Copy link

stackblitz bot commented Oct 25, 2023

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

@patak-dev
Copy link
Member

I think it would be better for @sapphi-red to review this one, but it looks good to me. I like moving away from connect-history-api-fallback, the new code looks easier to maintain than using the library.

@bluwy
Copy link
Member Author

bluwy commented Oct 26, 2023

Sapphi recommended to check with https://github.com/slorber/trailing-slash-guide, so here's the new comparison against it:

Host /file /file/ /file.html /folder /folder/ /folder/index.html /both /both/ /both.html /both/index.html
Vite ❌ (mpa 404) ❌ (mpa 404)

The main difference is that we don't really do redirects if a trailing slash is needed/missing. And we don't do rewrites which can be dangerous when there's relative paths in the html. So for the most part I think this PR is good.

@userquin
Copy link
Contributor

@bluwy your table looks very similar to this one : https://github.com/slorber/trailing-slash-guide

@bluwy
Copy link
Member Author

bluwy commented Oct 28, 2023

Yeah I linked it in the comment above

@bluwy
Copy link
Member Author

bluwy commented Oct 28, 2023

/ecosystem-ci run

@vite-ecosystem-ci

This comment was marked as outdated.

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

It looks mostly good to me.
Would you update this part of the migration guide?

### Allow path containing `.` to fallback to index.html
In Vite 4, accessing a path containing `.` did not fallback to index.html even if `appType` is set to `'SPA'` (default).
From Vite 5, it will fallback to index.html.
Note that the browser will no longer show the 404 error message in the console if you point the image path to a non-existent file (e.g. `<img src="./file-does-not-exist.png">`).

@@ -186,9 +189,25 @@ export async function preview(

Copy link
Member

Choose a reason for hiding this comment

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

Should we also align the base removal behavior? The dev server uses baseMiddleware that stips the base so that the middlewares doesn't have to take base into consideration.

// base
if (config.base !== '/') {
middlewares.use(baseMiddleware(server))
}

// rewrite url to remove base. this ensures that other middleware does
// not need to consider base being prepended or not

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'm not quite sure about this one. It would be a big breaking change I think for those expecting the base not removed in preview, and I think it's a bit late to make that change now 🤔

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 73e375b: Open

suite result latest scheduled
analogjs success success
astro failure success
histoire success success
ladle success success
laravel failure failure
marko failure failure
nuxt failure failure
nx failure failure
previewjs failure success
qwik success success
rakkas success success
sveltekit failure failure
unocss success success
vike failure success
vite-plugin-pwa success success
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc success success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress success success
vitest failure failure

@sapphi-red
Copy link
Member

Ah, the rerun posts a new message now.

Astro and Vike seems to be affected by this breaking change.

@sapphi-red sapphi-red added this to the 5.0 milestone Oct 29, 2023
@bluwy
Copy link
Member Author

bluwy commented Oct 30, 2023

The Astro fail is expected at the moment because it expects the post middlewares to have already handled serving HTML files: https://github.com/withastro/astro/blob/46d3043a3f41a826061c79241d54ecbd106e4ddd/packages/astro/src/core/preview/vite-plugin-astro-preview.ts#L53-L66. That can be workaround with a .html check first before running that middleware.

cc @brillout for the Vike fail

@sapphi-red
Copy link
Member

I found a different inconsistency between dev and preview.
When base: '/foo/' is set and /foo is accessed, dev will respond with 404 and preview will respond with a index.html.
But I think we can align that in a different PR later as it won't be a big change.

@bluwy
Copy link
Member Author

bluwy commented Oct 30, 2023

Alright I'll merge this first and we can try to fix that separately. Looks like we might need the baseMiddleware after-all if we want to align that. Even though I feel it's a bit late to change that now, maybe it's still worth a shot and see how ecosystem-ci reacts.

@bluwy bluwy merged commit 4f71ae8 into main Oct 30, 2023
10 checks passed
@bluwy bluwy deleted the align-html-dev-prod branch October 30, 2023 13:57
@brillout
Copy link
Contributor

cc @brillout for the Vike fail

It seems like every $ vite preview test is failing, so I'm guessing it breaks Vike's SSR middleware integration into Vite's preview server. I can have a look at it later today/tomorrow.

@bluwy
Copy link
Member Author

bluwy commented Oct 30, 2023

Thanks for the update @brillout. Sorry I had to merge before getting your input, as I'd like to unblock the continuing PRs and would like the finish up the 5.0 milestone soon. Let me know if you found any issues with it and I'll try to get it fixed.

@brillout
Copy link
Contributor

👍 No problems and, yes, I'll keep you udpated about my findings.

@brillout
Copy link
Contributor

@bluwy Seems like a bug: upon requesting /hello Vite doesn't look for the file /dist/hello.html nor dist/hello/index.html (with appType: 'custom').

@brillout
Copy link
Contributor

brillout commented Oct 31, 2023

I guess it's intentional because of appType: 'custom'. Edit: I consider it a bug, see comments in my PR #14836.

@dsine-de
Copy link

Does the latest vite version already come with this fix?
I get a nested HTML page only with a trailing slash with version 5.0.12.

@patak-dev
Copy link
Member

Yes, this PR has been released in Vite 5.0. Please create a new issue with a minimal reproduction. The comment here will be lost soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p3-significant High priority enhancement (priority)
Projects
None yet
6 participants