fix: appType mpa

[brillout]

Description

Fix appType: 'mpa' which currently doesn't work: indexHtmlMiddleware() doesn't work without spaFallbackMiddleware(). In other words MPAs also need spaFallbackMiddleware().

This seems to have been a glitch we missed at #8452.

Additional context

This is a blocker for Telefunc.

---

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.

[patak-dev]

Could you explain a bit more here @brillout? And we should add a test case too. I thought not having the spa fallback was one of the reasons to have appType: 'mpa'. cc @benmccann

[benmccann]

Why doesn't the index middleware work without the spa middleware and is that something we should fix?

[brillout]

The reason for 'mpa' was to avoid setting single: true for sirv(), which this PR preserves.

I unfortunately don't have the bandwidth to implement a test case.

I found a workaround, so this is not urgent/important from my side. But I do think we should fix this for other folks.

[patak-dev]

@brillout more than a test case, we need to understand why this is needed. I still don't get what are we fixing here.

[brillout]

indexHtmlMiddleware() only serves URLs ending with .html:

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

Line 291 in cc8800a

if (url?.endsWith('.html') && req.headers['sec-fetch-dest'] !== 'script') {

While spaFallbackMiddleware() appends '.html' to URLs:

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

Lines 17 to 24 in cc8800a

	const rewritten =
	decodeURIComponent(parsedUrl.pathname) + 'index.html'
	if (fs.existsSync(path.join(root, rewritten))) {
	return rewritten
	} else {
	return `/index.html`
	}

In other words:

Fix appType: 'mpa' which currently doesn't work: indexHtmlMiddleware() doesn't work without spaFallbackMiddleware(). In other words MPAs also need spaFallbackMiddleware().

I do agree that a proper fix would be to split out spaFallackMiddleware() in two middlewares (one that appends .html and a second one that does the SPA fallback). This fix is not perfect, but for sure better than the current situation.

[brillout]

I do agree that a proper fix would be to split out spaFallackMiddleware() in two middlewares (one that appends .html and a second one that does the SPA fallback). This fix is not perfect, but for sure better than the current situation.

Nevermind, I implemented another proper fix. See the latest commit.

I believe everything is fixed now.

[patak-dev]

@benmccann @matthewp @danielroe are you removing the spaFallbackMiddleware in SvelteKit, Astro, Nuxt using its function name? I would like to check if applying this fix in 3.1 is safe. If not, we will need to create another middleware, but that would also be breaking if you need to remove it. Another option is to wait until Vite 4, but this looks like a bug we should fix sooner.

[danielroe]

We are not using spaFallbackMiddleware in Nuxt 👍

[patak-dev]

@danielroe to clarify, it isnt about using it, but if you are manually removing it by function name. If you are removing this middleware, then your app could break as the name is going to change.

[matthewp]

We do remove that one in Astro, yes: https://github.com/withastro/astro/blob/fc32e2d94cf0eb19e5715055865d6bb200b8918c/packages/astro/src/vite-plugin-astro-server/index.ts#L31-L43

[patak-dev]

Thanks for the feedback @matthewp, I see you are now using appType: 'custom' everywhere, no?
https://github.com/withastro/astro/blob/ac03218247763e4782824e220a384fd20ae6d769/packages/astro/src/core/config.ts#L510

If that is the case, then you don't need to remove by hand this middleware anymore (from Vite 3). And you shouldn't be affected by this PR either. appType: 'custom' doesn't include any of the PRs in that list you linked. You can delete that part, it was one of the reasons to add 'custom' 👍🏼
Check the code that is adding the middlewares here

vite/packages/vite/src/node/server/index.ts

Line 544 in 2d2f2e5

if (config.appType === 'spa') {

[patak-dev]

@bluwy SvelteKit is also using appType: 'custom', no? I think we could merge this PR in 3.1 at the end

[bluwy]

Yup SvelteKit is using custom too.

[bluwy]

Just me being really nitpicky, but I feel like htmlFallbackMiddleware better reflects its usecase 🤔

[bluwy]

Perhaps we want to change the names here too.

[bluwy]

Few lines below we have:

  return function viteSpaFallbackMiddleware(req, res, next) {
    return historySpaFallbackMiddleware(req, res, next)
  }

so technically we're still preserving the old function name (so someone can remove it). If appType is the blessed way forward, we could say that the middleware function names aren't part of the public API following semver? And change it too? 🤔

[patak-dev]

Oh, I didn't see that the name was not yet changed.

Yes, I think we should rename it to htmlFallbackMiddleware in this PR, since everyone in v3 that doesn't want the HTML middleware is using appType: custom I think we could do this change in Vite 3.1.

[benmccann]

I just checked to confirm - we don't remove spaFallbackMiddleware anymore for SvelteKit now that there's the custom mode. The only ones we still have to remove are ['viteServePublicMiddleware', 'viteServeStaticMiddleware']

[jiby-aurum]

@patak-dev curious what is the status of this PR, as I see this is the last issue blocking 3.2.

[bluwy]

@jiby-aurum This PR needs a rebase and the naming changes discussed above. If you'd like to help out that'd be great! You can submit a PR to brillout's fork or open a new PR here with the updated changes.

[jiby-aurum]

@bluwy sure, happy to help. I opened a new PR with the updated changes #10336