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.

Sign up for GitHub

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

Jump to bottom

fix: appType mpa #9865

Closed
wants to merge 4 commits into from
Closed

fix: appType mpa #9865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a592eb
fix: appType mpa
brillout Aug 26, 2022
0d8320d
fix: only apply SPA fallback for SPA apps
brillout Aug 27, 2022
dfc396a
chore: improve naming
brillout Aug 27, 2022
85f8717
chore: improve comment
brillout Aug 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/vite/src/node/server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
9 changes: 6 additions & 3 deletions ...rc/node/server/middlewares/spaFallback.ts → ...src/node/server/middlewares/rewriteUrl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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