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: consistent html handling between dev and preview #11289

Closed
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
60 changes: 53 additions & 7 deletions packages/vite/src/node/preview.ts
Expand Up @@ -17,8 +17,11 @@ import compression from './server/middlewares/compression'
import { proxyMiddleware } from './server/middlewares/proxy'
import { resolveHostname, resolveServerUrls, shouldServe } from './utils'
import { printServerUrls } from './logger'
import { resolveConfig } from '.'
import { htmlFallbackMiddleware } from './server/middlewares/htmlFallback'
import { indexHtmlPreviewMiddleware } from './server/middlewares/indexHtml'
import { notFoundMiddleware } from './server/middlewares/notFound'
import type { InlineConfig, ResolvedConfig } from '.'
import { resolveConfig } from '.'

export interface PreviewOptions extends CommonServerOptions {}

Expand Down Expand Up @@ -120,7 +123,7 @@ export async function preview(
const assetServer = sirv(distDir, {
etag: true,
dev: true,
single: config.appType === 'spa',
extensions: [],
setHeaders(res) {
if (headers) {
for (const name in headers) {
Expand All @@ -129,16 +132,59 @@ export async function preview(
}
},
})
app.use(previewBase, async (req, res, next) => {
if (shouldServe(req.url!, distDir)) {
return assetServer(req, res, next)
app.use(
previewBase,
// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
async function vitePreviewStaticMiddleware(req, res, next) {
if (shouldServe(req.url!, distDir)) {
return assetServer(req, res, next)
}
next()
},
)

// html fallback
if (config.appType === 'spa' || config.appType === 'mpa') {
// append trailing slash when base didn't have it
if (config.rawBase !== config.base) {
// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
app.use(function viteRewriteBaseAccessWithoutTrailingSlashMiddleware(
req,
res,
next,
) {
try {
const pathname = decodeURIComponent(
new URL(req.url!, 'http://example.com').pathname,
)
if (pathname === config.rawBase) {
req.url = config.base + req.url!.slice(config.rawBase.length)
benmccann marked this conversation as resolved.
Show resolved Hide resolved
}
} catch {}
next()
})
}
next()
})

app.use(
benmccann marked this conversation as resolved.
Show resolved Hide resolved
previewBase,
htmlFallbackMiddleware(distDir, config.appType === 'spa'),
)
Copy link
Member Author

Choose a reason for hiding this comment

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

We could add a middleware here to support rewriting /foo to /foo.html. I didn't implement it because it wasn't supported in dev.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to add an option in preview to skip the shouldServe and let sirv handle it?.

That would allow frameworks like îles to configure the preview to work in a way that is consistent with the deployment target (which might resolve /foo to /foo.html).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that is possible by injecting a middleware by the post configurePreviewServer hook, too.
In that way, I think we can have the same behavior in dev by injecting that middleware by configureServer hook.

}

// apply post server hooks from plugins
postHooks.forEach((fn) => fn && fn())

if (config.appType === 'spa' || config.appType === 'mpa') {
// serve index.html
app.use(
previewBase,
indexHtmlPreviewMiddleware(distDir, config.preview.headers),
)

// handle 404s
app.use(previewBase, notFoundMiddleware())
}

const options = config.preview
const hostname = await resolveHostname(options.host)
const port = options.port ?? 4173
Expand Down
7 changes: 2 additions & 5 deletions packages/vite/src/node/server/index.ts
Expand Up @@ -77,6 +77,7 @@ import { openBrowser } from './openBrowser'
import type { TransformOptions, TransformResult } from './transformRequest'
import { transformRequest } from './transformRequest'
import { searchForWorkspaceRoot } from './searchRoot'
import { notFoundMiddleware } from './middlewares/notFound'

export { searchForWorkspaceRoot } from './searchRoot'

Expand Down Expand Up @@ -600,11 +601,7 @@ export async function createServer(
middlewares.use(indexHtmlMiddleware(server))

// handle 404s
// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
middlewares.use(function vite404Middleware(_, res) {
res.statusCode = 404
res.end()
})
middlewares.use(notFoundMiddleware())
}

// error handler
Expand Down
30 changes: 30 additions & 0 deletions packages/vite/src/node/server/middlewares/indexHtml.ts
@@ -1,5 +1,6 @@
import fs from 'node:fs'
import path from 'node:path'
import type { OutgoingHttpHeaders } from 'node:http'
import MagicString from 'magic-string'
import type { SourceMapInput } from 'rollup'
import type { Connect } from 'dep-types/connect'
Expand Down Expand Up @@ -303,3 +304,32 @@ export function indexHtmlMiddleware(
next()
}
}

export function indexHtmlPreviewMiddleware(
root: string,
headers: OutgoingHttpHeaders | undefined,
): Connect.NextHandleFunction {
// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
return async function viteIndexHtmlPreviewMiddleware(req, res, next) {
if (res.writableEnded) {
return next()
}

const url = req.url && cleanUrl(req.url)
// htmlFallbackMiddleware appends '.html' to URLs
if (url?.endsWith('.html')) {
const filepath = path.join(root, url.slice(1))
if (fs.existsSync(filepath)) {
try {
const html = fs.readFileSync(filepath, 'utf-8')
return send(req, res, html, 'html', {
headers,
})
} catch (e) {
return next(e)
}
}
}
next()
}
}
10 changes: 10 additions & 0 deletions packages/vite/src/node/server/middlewares/notFound.ts
@@ -0,0 +1,10 @@
import type { Connect } from 'dep-types/connect'

// handle 404s
export function notFoundMiddleware(): Connect.NextHandleFunction {
// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
return function vite404Middleware(_, res) {
res.statusCode = 404
res.end()
}
}
55 changes: 49 additions & 6 deletions playground/assets/__tests__/assets.spec.ts
Expand Up @@ -24,19 +24,62 @@ const assetMatch = isBuild

const iconMatch = `/foo/icon.png`

const fetchPath = (p: string) => {
return fetch(path.posix.join(viteTestUrl, p), {
headers: { Accept: 'text/html,*/*' },
})
}

test('should have no 404s', () => {
browserLogs.forEach((msg) => {
expect(msg).not.toMatch('404')
})
})

test('should get a 404 when using incorrect case', async () => {
expect((await fetch(path.posix.join(viteTestUrl, 'icon.png'))).status).toBe(
200,
)
expect((await fetch(path.posix.join(viteTestUrl, 'ICON.png'))).status).toBe(
404,
)
expect((await fetchPath('icon.png')).status).toBe(200)
// won't be wrote to index.html because the url includes `.`
expect((await fetchPath('ICON.png')).status).toBe(404)

expect((await fetchPath('bar')).status).toBe(200)
// fallback to index.html
const incorrectBarFetch = await fetchPath('BAR')
expect(incorrectBarFetch.status).toBe(200)
expect(incorrectBarFetch.headers.get('Content-Type')).toBe('text/html')
})

test('html resolve behavior', async () => {
const [
nestedIndexHtml,
nested,
nestedSlash,

nonNestedHtml,
nonNested,
nonNestedSlash,
] = await Promise.all([
fetchPath('nested/index.html'), // -> nested/index.html
fetchPath('nested'), // -> index.html
fetchPath('nested/'), // -> nested/index.html

fetchPath('non-nested.html'), // -> non-nested.html
fetchPath('non-nested'), // -> index.html
fetchPath('non-nested/'), // -> index.html
Comment on lines +61 to +67
Copy link
Member Author

@sapphi-red sapphi-red Dec 9, 2022

Choose a reason for hiding this comment

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

With dev server, these tests passes without this PR.
With preview server and without this PR, these tests resolved to

])

expect(nestedIndexHtml.status).toBe(200)
expect(await nestedIndexHtml.text()).toContain('nested html')
expect(nested.status).toBe(200)
expect(await nested.text()).toContain('Assets')
expect(nestedSlash.status).toBe(200)
expect(await nestedSlash.text()).toContain('nested html')

expect(nonNestedHtml.status).toBe(200)
expect(await nonNestedHtml.text()).toContain('non-nested html')
expect(nonNested.status).toBe(200)
expect(await nonNested.text()).toContain('Assets')
expect(nonNestedSlash.status).toBe(200)
expect(await nonNestedSlash.text()).toContain('Assets')
})

describe('injected scripts', () => {
Expand Down
1 change: 1 addition & 0 deletions playground/assets/nested/index.html
@@ -0,0 +1 @@
<div>nested html</div>
1 change: 1 addition & 0 deletions playground/assets/non-nested.html
@@ -0,0 +1 @@
<div>non-nested html</div>
1 change: 1 addition & 0 deletions playground/assets/static/bar
@@ -0,0 +1 @@
bar
11 changes: 10 additions & 1 deletion playground/assets/vite.config.js
@@ -1,5 +1,7 @@
const path = require('node:path')

const resolve = (p) => path.resolve(__dirname, p)

/**
* @type {import('vite').UserConfig}
*/
Expand All @@ -8,7 +10,7 @@ module.exports = {
publicDir: 'static',
resolve: {
alias: {
'@': path.resolve(__dirname, 'nested'),
'@': resolve('nested'),
},
},
assetsInclude: ['**/*.unknown'],
Expand All @@ -17,5 +19,12 @@ module.exports = {
assetsInlineLimit: 8192, // 8kb
manifest: true,
watch: {},
rollupOptions: {
input: [
resolve('./index.html'),
resolve('./nested/index.html'),
resolve('./non-nested.html'),
],
},
},
}