fix: consistent html handling between dev and preview

[sapphi-red]

Description

I thought it difficult to use sirv's single: true and extensions feature with shouldServe. We could use those, but we will still need to implement those behaviors to shouldServe on our side.
So instead of relying on sirv, this PR makes preview server to use the same middlewares with the dev server.

This is a possible breaking change, because the preview server's behavior is now aligned to the dev server's behavior. For example, /foo won't be resolved to /foo.html.

fixes #6596
fixes #10925
superseds close #10944

Additional context

---

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 Commit 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.

[sapphi-red]

We could add a middleware here to support rewriting /foo to /foo.html. I didn't implement it because it wasn't supported in dev.

[ElMassimo]

Would it be possible to add an option in preview to skip the shouldServe and let sirv handle it?.

That would allow frameworks like îles to configure the preview to work in a way that is consistent with the deployment target (which might resolve /foo to /foo.html).

[sapphi-red]

I think that is possible by injecting a middleware by the post configurePreviewServer hook, too.
In that way, I think we can have the same behavior in dev by injecting that middleware by configureServer hook.

[sapphi-red]

With dev server, these tests passes without this PR.
With preview server and without this PR, these tests resolved to

• nested/index.html -> nested/index.html
• nested -> nested/index.html (changed, Inconsistent URL trailing slash behavior between dev and preview servers #6596)
• nested/ -> nested/index.html
• non-nested.html -> non-nested.html
• non-nested -> 404 (changed, Routing broken for preview with react-router-dom in 4.0.0-alpha.2 #10925)
• non-nested/ -> 404 (changed, Routing broken for preview with react-router-dom in 4.0.0-alpha.2 #10925)

[patak-dev]

Interesting, I agree that the extra complexity is worth it. @benmccann @bluwy @ElMassimo @brillout would you help us review this PR? It is a big change for a patch but given the regression and that it only affects preview, I think we should move forward with it as soon as we agree this is good for all of you.

[benmccann]

lgtm as long as we can run the ecosystem CI against this before merge and it passes our tests. sorry for causing the regression here and thank you so much for investigating and fixing it!! ❤️ 💯

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
astro	✅ success
histoire	✅ success
iles	❌ failure
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt-framework	❌ failure
previewjs	✅ success
rakkas	✅ success
sveltekit	✅ success
vite-plugin-ssr	❌ failure
vite-plugin-react	✅ success
vite-plugin-react-swc	✅ success
vite-plugin-svelte	✅ success
vite-plugin-vue	✅ success
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success
windicss	✅ success

[bluwy]

Would it be simpler if we patch sirv to enforce case sensitivity instead? This seems to add a lot of dev middlewares into preview, where I believe the goal of preview is to be a simple static file server.

We could probably add the case-sensitive check here by exposing another option. Also it seems like for dev: false it enforces strict case-sensitivity by default? (not tested)

[sapphi-red]

Would it be simpler if we patch sirv to enforce case sensitivity instead?

I don't think patching is a good thing in terms of maintainability for the long run, given that case sensitivity feature might not land in sirv (lukeed/sirv#141 (comment)).

This seems to add a lot of dev middlewares into preview, where I believe the goal of preview is to be a simple static file server.

If we wanted to align the preview and dev server behavior, I think we cannot avoid this complexity.

[bluwy]

I don't think the preview server needs to align with the dev server. The preview server should be allowed to implement a superset of what dev supports to mimic production behaviour, so I'm not sure if the breaking change is needed.

sirv is quite a low-maintenance package so I think it's still acceptable to make a patch. I was also thinking we can upstream this feature as { shouldServe: () => {} } instead of { enforceCaseSensitivity: true } so it's more generic.

[patak-dev]

The last commit to sirv was one year ago. I agree with @bluwy that keeping a patch for it shouldn't bring much maintenance overhead for us. We may also want to check out alternatives to sirv. There aren't many issues, but @sapphi-red issue hasn't been addressed for example lukeed/sirv#138.

[brillout]

For example, /foo won't be resolved to /foo.html.

That's a must-have for SSGs (i.e. pre-rendered apps), since the HTML is served statically from dist/client/. For example, the HTML of /about is generated at build-time and lives at dist/client/about.html. Consequently, Vite needs to append .html. That's, btw., the reason why VPS (vite-plugin-ssr) fails against this PR.

As for the rest of the PR, I lack the context to provide feedback. That said, further aligning the $ vite dev and $ vite preview sounds good to me.

I believe the goal of preview is to be a simple static file server.

Since configurePreviewServer(), that's not the case anymore. For example, VPS injects its SSR middleware to the preview server. I believe SvelteKit does this as well. And I've read about some Vite end-users doing this also.

On a high level I'd say: $ vite preview should behave as close as possible to $ vite dev without all dev-only capabilities such as HMR.

[benmccann]

I was also thinking we can upstream this feature as { shouldServe: () => {} } instead of { enforceCaseSensitivity: true } so it's more generic.

Does that help? I think sirv would need to pass the location on disk where it found the file as a parameter or else we'd still end up re-implementing single: true and extensions

I agree with @bluwy that keeping a patch for it shouldn't bring much maintenance overhead for us.

sirv's a single file and is less than 200 lines, so we could copy it into the project and modify it as needed.

We may also want to check out alternatives to sirv.

I guess this would be the main one? http://expressjs.com/en/resources/middleware/serve-static.html

It has an index option to serve index.html files, but I don't see an option to serve about.html for a request to /about, so I'm guessing it doesn't do that

There aren't many issues, but @sapphi-red issue hasn't been addressed for example lukeed/sirv#138.

That looks like an easy one. I just sent a PR: lukeed/sirv#149

That's, btw., the reason why VPS (vite-plugin-ssr) fails against that PR.

@brillout to confirm, when you say "that PR", do you mean this PR?

[bluwy]

Does that help? I think sirv would need to pass the location on disk where it found the file as a parameter or else we'd still end up re-implementing single: true and extensions

Yeah I was on my phone so I omitted the params, but it should be like shouldServe: (filePath: string) => boolean

[patak-dev]

I agree with @bluwy that keeping a patch for it shouldn't bring much maintenance overhead for us.

sirv's a single file and is less than 200 lines, so we could copy it into the project and modify it as needed.

This sounds good to me. Less complexity than keeping the patch, and no maintainance overhead of external users asking for features. And if later there are a lot of changes in sirv and becomes active we could go back to use it as a dep instead of tracking them. I think we will also benefit by having the code in ts directly explorable in Vite's source.

[sapphi-red]

I've created a PR that patches sirv (#11312). I think we should go with #11312, while we discuss about this PR.

I still think it would be nice to align preview and dev server behavior. But I can understand that the complexity might not worth it.

Also I think having shouldServe will clean this part and will be more safe for things like #8498.

vite/packages/vite/src/node/server/middlewares/static.ts

Lines 137 to 148 in 42976d8

	const pathname = decodeURIComponent(url.pathname)
	// restrict files outside of `fs.allow`
	if (
	!ensureServingAccess(
	slash(path.resolve(fsPathFromId(pathname))),
	server,
	res,
	next,
	)
	) {
	return
	}

[brillout]

@brillout to confirm, when you say "that PR", do you mean this PR?

Yes

[brillout]

There is another regression: #11346.

[bluwy]

Do we still need to keep this PR given we've merged #11312?

[sapphi-red]

I still think it would be nice to align the dev/preview server behavior.
But given that it is considered that the complexity isn't worth it, I now agree that should be addressed in a different way. So I'll close this one 👍