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(server): handle appType mpa html fallback #10336

Merged
merged 2 commits into from Oct 5, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba3898e
fix: appType mpa
jiby-aurum Oct 3, 2022
a43b021
chore: fixes
bluwy Oct 4, 2022
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
8 changes: 4 additions & 4 deletions packages/vite/src/node/server/index.ts
View file
Open in desktop
Expand Up @@ -53,7 +53,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 { htmlFallbackMiddleware } from './middlewares/htmlFallback'
import { transformMiddleware } from './middlewares/transform'
import {
createDevHtmlTransformFn,
Expand Down Expand Up @@ -560,9 +560,9 @@ export async function createServer(
middlewares.use(serveRawFsMiddleware(server))
middlewares.use(serveStaticMiddleware(root, server))

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

// run post config hooks
Expand Down
17 changes: 10 additions & 7 deletions ...rc/node/server/middlewares/spaFallback.ts → ...c/node/server/middlewares/htmlFallback.ts
View file
Open in desktop
Expand Up @@ -4,11 +4,12 @@ import history from 'connect-history-api-fallback'
import type { Connect } from 'dep-types/connect'
import { createDebugger } from '../../utils'

export function spaFallbackMiddleware(
root: string
export function htmlFallbackMiddleware(
root: string,
spaFallback: boolean
): Connect.NextHandleFunction {
const historySpaFallbackMiddleware = history({
logger: createDebugger('vite:spa-fallback'),
const historyHtmlFallbackMiddleware = history({
logger: createDebugger('vite:html-fallback'),
// support /dir/ without explicit index.html
rewrites: [
{
Expand All @@ -20,15 +21,17 @@ export function spaFallbackMiddleware(
if (fs.existsSync(path.join(root, rewritten))) {
return rewritten
} else {
return `/index.html`
if (spaFallback) {
return `/index.html`
}
Comment on lines +24 to +26
Copy link

Choose a reason for hiding this comment

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

I'm getting errors when setting appType: 'mpa' and requesting a URI from the dev server that doesn't exist (and would fall back to index.html in the SPA case). Instead of returning a simple 404, as expected, it's a hmr overlay that says:

Cannot read properties of undefined (reading 'charAt')

    at file:///usr/src/client/node_modules/vite/dist/node/chunks/dep-51c4f80a.js:59865:27
    at viteHtmlFallbackMiddleware (file:///usr/src/client/node_modules/vite/dist/node/chunks/dep-51c4f80a.j

I couldn't track back the code in that chunk to it's source (not sure which package this is from?), but it does this:

if(rewriteTarget.charAt(0) !== '/') {
	          logger(
	            'We recommend using an absolute path for the rewrite target.',
	            'Received a non-absolute rewrite target',
	            rewriteTarget,
	            'for URL',
	            req.url
	          );
	        }

Clearly, it expects the rewrite to return a string in any case. But the lines I commented on, will return undefined for mpa.

I can fix this by just returning an empty string in the mpa case. I'm not sure whether that's the right thing to do, though?

Commenting here, because this change introduced the bug.

Copy link
Member

Choose a reason for hiding this comment

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

Would you create a minimal reproduction and an issue in the repo so we don't lose track of this issue? You can link to this PR and comment, that is helpful to know already the connection.

Copy link

Choose a reason for hiding this comment

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

Done in #10966.

}
}
}
]
})

// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
return function viteSpaFallbackMiddleware(req, res, next) {
return historySpaFallbackMiddleware(req, res, next)
return function viteHtmlFallbackMiddleware(req, res, next) {
return historyHtmlFallbackMiddleware(req, res, next)
}
}
2 changes: 1 addition & 1 deletion packages/vite/src/node/server/middlewares/indexHtml.ts
View file
Open in desktop
Expand Up @@ -283,7 +283,7 @@ export function indexHtmlMiddleware(
}

const url = req.url && cleanUrl(req.url)
// spa-fallback always redirects to /index.html
// htmlFallbackMiddleware 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