fix: appType mpa by brillout · Pull Request #9865 · vitejs/vite

brillout · 2022-08-26T12:42:08Z

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-cat · 2022-08-26T12:46:07Z

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 · 2022-08-26T12:53:28Z

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

brillout · 2022-08-26T13:01:06Z

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-cat · 2022-08-26T15:02:56Z

@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 · 2022-08-27T08:54:43Z

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 · 2022-08-27T10:49:01Z

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-cat · 2022-08-28T18:33:10Z

@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 · 2022-08-29T08:12:50Z

We are not using spaFallbackMiddleware in Nuxt 👍

patak-cat · 2022-08-29T08:30:39Z

@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 · 2022-08-29T12:10:18Z

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-cat · 2022-08-29T12:16:19Z

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-cat · 2022-08-29T12:28:17Z

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

bluwy · 2022-08-29T13:00:29Z

Yup SvelteKit is using custom too.

bluwy · 2022-08-29T13:02:27Z

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


-export function spaFallbackMiddleware(
-  root: string
+export function rewriteUrlMiddleware(


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

bluwy · 2022-08-29T13:02:50Z

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

  const historySpaFallbackMiddleware = history({
    logger: createDebugger('vite:spa-fallback'),


Perhaps we want to change the names here too.

bluwy · 2022-08-29T13:05:37Z

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

+            }
          }
        }
      }


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? 🤔

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 · 2022-08-29T13:35:53Z

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']

gb-jos · 2022-10-03T07:44:44Z

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

bluwy · 2022-10-03T17:02:52Z

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

gb-jos · 2022-10-03T19:58:39Z

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

fix: appType mpa

7a592eb

fix: only apply SPA fallback for SPA apps

0d8320d

brillout force-pushed the fix/appType-map branch from 7ef90fc to d170896 Compare August 27, 2022 09:17

chore: improve naming

dfc396a

brillout force-pushed the fix/appType-map branch from d170896 to dfc396a Compare August 27, 2022 10:47

chore: improve comment

85f8717

brillout mentioned this pull request Aug 27, 2022

Pr use ts for vite example telefunc/telefunc#28

Merged

patak-cat added p3-minor-bug An edge case that only affects very specific usage (priority) feat: html labels Aug 28, 2022

bluwy reviewed Aug 29, 2022

View reviewed changes

bluwy mentioned this pull request Aug 29, 2022

Remove hardcoded Vite middleware handling withastro/astro#4531

Merged

patak-cat added this to the 3.2 milestone Sep 3, 2022

gb-jos mentioned this pull request Oct 3, 2022

fix(server): handle appType mpa html fallback #10336

Merged

9 tasks

patak-cat closed this in #10336 Oct 5, 2022

bluwy mentioned this pull request Sep 8, 2023

feat!: allow path containing . to fallback to index.html #14142

Merged

9 tasks

brillout deleted the fix/appType-map branch September 18, 2024 08:14

		const historySpaFallbackMiddleware = history({
		logger: createDebugger('vite:spa-fallback'),

Uh oh!

Conversation

brillout commented Aug 26, 2022

Description

Additional context

What is the purpose of this pull request?

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

Uh oh!

patak-cat commented Aug 26, 2022

Uh oh!

benmccann commented Aug 26, 2022

Uh oh!

brillout commented Aug 26, 2022

Uh oh!

patak-cat commented Aug 26, 2022

Uh oh!

brillout commented Aug 27, 2022

Uh oh!

brillout commented Aug 27, 2022

Uh oh!

patak-cat commented Aug 28, 2022

Uh oh!

danielroe commented Aug 29, 2022

Uh oh!

patak-cat commented Aug 29, 2022

Uh oh!

matthewp commented Aug 29, 2022

Uh oh!

patak-cat commented Aug 29, 2022

Uh oh!

patak-cat commented Aug 29, 2022

Uh oh!

bluwy commented Aug 29, 2022

Uh oh!

bluwy Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

bluwy Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

bluwy Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

patak-cat Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

benmccann commented Aug 29, 2022

Uh oh!

gb-jos commented Oct 3, 2022

Uh oh!

bluwy commented Oct 3, 2022

Uh oh!

gb-jos commented Oct 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants