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: appType mpa #9865

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/vite/src/node/server/index.ts
Expand Up @@ -50,7 +50,7 @@ import type { WebSocketServer } from './ws'
import { createWebSocketServer } from './ws'
import { baseMiddleware } from './middlewares/base'
import { proxyMiddleware } from './middlewares/proxy'
import { spaFallbackMiddleware } from './middlewares/spaFallback'
import { rewriteUrlMiddleware } from './middlewares/rewriteUrl'
import { transformMiddleware } from './middlewares/transform'
import {
createDevHtmlTransformFn,
Expand Down Expand Up @@ -542,8 +542,8 @@ export async function createServer(
middlewares.use(serveStaticMiddleware(root, server))

// spa fallback
if (config.appType === 'spa') {
middlewares.use(spaFallbackMiddleware(root))
if (config.appType === 'spa' || config.appType === 'mpa') {
middlewares.use(rewriteUrlMiddleware(root, config.appType === 'spa'))
}

// run post config hooks
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/server/middlewares/indexHtml.ts
Expand Up @@ -287,7 +287,7 @@ export function indexHtmlMiddleware(
}

const url = req.url && cleanUrl(req.url)
// spa-fallback always redirects to /index.html
// rewriteUrlMiddleware() appends '.html' to URLs
if (url?.endsWith('.html') && req.headers['sec-fetch-dest'] !== 'script') {
const filename = getHtmlFilename(url, server)
if (fs.existsSync(filename)) {
Expand Down
Expand Up @@ -4,8 +4,9 @@ import history from 'connect-history-api-fallback'
import type { Connect } from 'types/connect'
import { createDebugger } from '../../utils'

export function spaFallbackMiddleware(
root: string
export function rewriteUrlMiddleware(
Copy link
Member

Choose a reason for hiding this comment

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

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

root: string,
spaFallback: boolean
): Connect.NextHandleFunction {
const historySpaFallbackMiddleware = history({
logger: createDebugger('vite:spa-fallback'),
Comment on lines 11 to 12
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we want to change the names here too.

Expand All @@ -20,7 +21,9 @@ export function spaFallbackMiddleware(
if (fs.existsSync(path.join(root, rewritten))) {
return rewritten
} else {
return `/index.html`
if (spaFallback) {
return `/index.html`
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Expand Down