-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
feat(ssr): support for ssr.resolve.conditions and ssr.resolve.externalConditions options #14498
Changes from 3 commits
96a4c55
20e890e
399d824
7b225ec
3fc8cc1
72c0e5c
e6b20fb
4b4912c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -123,19 +123,23 @@ async function instantiateModule( | |||||||||||||
isProduction, | ||||||||||||||
resolve: { dedupe, preserveSymlinks }, | ||||||||||||||
root, | ||||||||||||||
ssr, | ||||||||||||||
} = server.config | ||||||||||||||
|
||||||||||||||
const overrideConditions = ssr.resolve?.externalConditions || [] | ||||||||||||||
|
||||||||||||||
const resolveOptions: InternalResolveOptionsWithOverrideConditions = { | ||||||||||||||
mainFields: ['main'], | ||||||||||||||
browserField: true, | ||||||||||||||
conditions: [], | ||||||||||||||
overrideConditions: ['production', 'development'], | ||||||||||||||
overrideConditions: [...overrideConditions, 'production', 'development'], | ||||||||||||||
extensions: ['.js', '.cjs', '.json'], | ||||||||||||||
dedupe, | ||||||||||||||
preserveSymlinks, | ||||||||||||||
isBuild: false, | ||||||||||||||
isProduction, | ||||||||||||||
root, | ||||||||||||||
ssrConfig: ssr, | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
// Since dynamic imports can happen in parallel, we need to | ||||||||||||||
|
@@ -271,6 +275,8 @@ async function nodeImport( | |||||||||||||
if (id.startsWith('data:') || isBuiltin(id)) { | ||||||||||||||
url = id | ||||||||||||||
} else { | ||||||||||||||
const targetWeb = resolveOptions.ssrConfig?.target === 'webworker' | ||||||||||||||
|
||||||||||||||
const resolved = tryNodeResolve( | ||||||||||||||
id, | ||||||||||||||
importer, | ||||||||||||||
|
@@ -280,7 +286,9 @@ async function nodeImport( | |||||||||||||
typeof jest === 'undefined' | ||||||||||||||
? { ...resolveOptions, tryEsmOnly: true } | ||||||||||||||
: resolveOptions, | ||||||||||||||
false, | ||||||||||||||
targetWeb, | ||||||||||||||
undefined, | ||||||||||||||
true, | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Because this part imitates Node and Node's resolver works like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, but afaik a main use case for Although I suppose the end user can now achieve this same result by explicitly listing the relevant conditions in Since there's another way to achieve the desired result via the new ssr.resolve options, I can change it back to false, just please confirm with a 👍 when you have a sec. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If a user is going to create a bundle for non-node runtimes, we expect them to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, but I still think there are some rough edges when trying to use vite createServer + ssrLoadModule to create a nice local dev server experience (in conjunction w actually running dev server in non node runtimes like bun, or when wanting to better represent the final build target during local dev when that build target is non-node like cloudflare). BUT I think best as a separate discussion, the adjustments in this PR make it possible to cover everything :). I reverted the targetWeb change - thanks for the review! |
||||||||||||||
) | ||||||||||||||
if (!resolved) { | ||||||||||||||
const err: any = new Error( | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// this is automatically detected by playground/vitestSetup.ts and will replace | ||
// the default e2e test serve behavior | ||
|
||
import path from 'node:path' | ||
import kill from 'kill-port' | ||
import { hmrPorts, ports, rootDir } from '~utils' | ||
|
||
export const port = ports['ssr-conditions'] | ||
|
||
export async function serve(): Promise<{ close(): Promise<void> }> { | ||
await kill(port) | ||
|
||
const { createServer } = await import(path.resolve(rootDir, 'server.js')) | ||
const { app, vite } = await createServer(rootDir, hmrPorts['ssr-conditions']) | ||
|
||
return new Promise((resolve, reject) => { | ||
try { | ||
const server = app.listen(port, () => { | ||
resolve({ | ||
// for test teardown | ||
async close() { | ||
await new Promise((resolve) => { | ||
server.close(resolve) | ||
}) | ||
if (vite) { | ||
await vite.close() | ||
} | ||
}, | ||
}) | ||
}) | ||
} catch (e) { | ||
reject(e) | ||
} | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import { expect, test } from 'vitest' | ||
import { port } from './serve' | ||
import { page } from '~utils' | ||
|
||
const url = `http://localhost:${port}` | ||
|
||
test('ssr.resolve.conditions affect non-externalized imports during ssr', async () => { | ||
await page.goto(url) | ||
expect(await page.textContent('.no-external-react-server')).toMatch( | ||
'node.unbundled.js', | ||
) | ||
}) | ||
|
||
test('ssr.resolve.externalConditions affect externalized imports during ssr', async () => { | ||
await page.goto(url) | ||
expect(await page.textContent('.external-react-server')).toMatch('edge.js') | ||
}) | ||
|
||
test('ssr.resolve settings do not affect non-ssr imports', async () => { | ||
await page.goto(url) | ||
expect(await page.textContent('.browser-no-external-react-server')).toMatch( | ||
'default.js', | ||
) | ||
expect(await page.textContent('.browser-external-react-server')).toMatch( | ||
'default.js', | ||
) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'browser.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'default.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'edge.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'node.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'node.unbundled.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"name": "@vitejs/test-ssr-conditions-external", | ||
"private": true, | ||
"version": "0.0.0", | ||
"type": "module", | ||
"exports": { | ||
"./server": { | ||
"react-server": { | ||
"workerd": "./edge.js", | ||
"deno": "./browser.js", | ||
"node": { | ||
"webpack": "./node.js", | ||
"default": "./node.unbundled.js" | ||
}, | ||
"edge-light": "./edge.js", | ||
"browser": "./browser.js" | ||
}, | ||
"default": "./default.js" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<!doctype html> | ||
<html lang="en"> | ||
<head> | ||
<meta charset="UTF-8" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
<title>SSR Resolve Conditions</title> | ||
</head> | ||
<body> | ||
<h1>SSR Resolve Conditions</h1> | ||
<div id="app"><!--app-html--></div> | ||
|
||
<script type="module"> | ||
import('@vitejs/test-ssr-conditions-no-external/server').then( | ||
({ default: message }) => { | ||
document.querySelector( | ||
'.browser-no-external-react-server', | ||
).textContent = message | ||
}, | ||
) | ||
|
||
import('@vitejs/test-ssr-conditions-external/server').then( | ||
({ default: message }) => { | ||
document.querySelector('.browser-external-react-server').textContent = | ||
message | ||
}, | ||
) | ||
</script> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'browser.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'default.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'edge.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'node.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'node.unbundled.js' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"name": "@vitejs/test-ssr-conditions-no-external", | ||
"private": true, | ||
"version": "0.0.0", | ||
"type": "module", | ||
"exports": { | ||
"./server": { | ||
"react-server": { | ||
"workerd": "./edge.js", | ||
"deno": "./browser.js", | ||
"node": { | ||
"webpack": "./node.js", | ||
"default": "./node.unbundled.js" | ||
}, | ||
"edge-light": "./edge.js", | ||
"browser": "./browser.js" | ||
}, | ||
"default": "./default.js" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
{ | ||
"name": "@vitejs/test-ssr-conditions", | ||
"private": true, | ||
"version": "0.0.0", | ||
"type": "module", | ||
"scripts": { | ||
"dev": "node server", | ||
"serve": "NODE_ENV=production node server", | ||
"debug": "node --inspect-brk server" | ||
}, | ||
"dependencies": { | ||
"@vitejs/test-ssr-conditions-external": "file:./external", | ||
"@vitejs/test-ssr-conditions-no-external": "file:./no-external" | ||
}, | ||
"devDependencies": { | ||
"express": "^4.18.2" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
import fs from 'node:fs' | ||
import path from 'node:path' | ||
import { fileURLToPath } from 'node:url' | ||
import express from 'express' | ||
|
||
const __dirname = path.dirname(fileURLToPath(import.meta.url)) | ||
|
||
const isTest = process.env.VITEST | ||
|
||
export async function createServer(root = process.cwd(), hmrPort) { | ||
const resolve = (p) => path.resolve(__dirname, p) | ||
|
||
const app = express() | ||
|
||
/** | ||
* @type {import('vite').ViteDevServer} | ||
*/ | ||
const vite = await ( | ||
await import('vite') | ||
).createServer({ | ||
root, | ||
logLevel: isTest ? 'error' : 'info', | ||
server: { | ||
middlewareMode: true, | ||
watch: { | ||
// During tests we edit the files too fast and sometimes chokidar | ||
// misses change events, so enforce polling for consistency | ||
usePolling: true, | ||
interval: 100, | ||
}, | ||
hmr: { | ||
port: hmrPort, | ||
}, | ||
}, | ||
appType: 'custom', | ||
}) | ||
|
||
app.use(vite.middlewares) | ||
|
||
app.use('*', async (req, res) => { | ||
try { | ||
const url = req.originalUrl | ||
|
||
let template | ||
template = fs.readFileSync(resolve('index.html'), 'utf-8') | ||
template = await vite.transformIndexHtml(url, template) | ||
const render = (await vite.ssrLoadModule('/src/app.js')).render | ||
|
||
const appHtml = await render(url, __dirname) | ||
|
||
const html = template.replace(`<!--app-html-->`, appHtml) | ||
|
||
res.status(200).set({ 'Content-Type': 'text/html' }).end(html) | ||
} catch (e) { | ||
vite && vite.ssrFixStacktrace(e) | ||
console.log(e.stack) | ||
res.status(500).end(e.stack) | ||
} | ||
}) | ||
|
||
return { app, vite } | ||
} | ||
|
||
if (!isTest) { | ||
createServer().then(({ app }) => | ||
app.listen(5173, () => { | ||
console.log('http://localhost:5173') | ||
}), | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import noExternalReactServerMessage from '@vitejs/test-ssr-conditions-no-external/server' | ||
import externalReactServerMessage from '@vitejs/test-ssr-conditions-external/server' | ||
|
||
export async function render(url) { | ||
let html = '' | ||
|
||
html += `\n<p class="no-external-react-server">${noExternalReactServerMessage}</p>` | ||
|
||
html += `\n<p class="browser-no-external-react-server"></p>` | ||
|
||
html += `\n<p class="external-react-server">${externalReactServerMessage}</p>` | ||
|
||
html += `\n<p class="browser-external-react-server"></p>` | ||
|
||
return html + '\n' | ||
} |
marbemac marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import noExternalReactServerMessage from '@vitejs/test-ssr-conditions-no-external/server' | ||
import externalReactServerMessage from '@vitejs/test-ssr-conditions-external/server' | ||
|
||
export { noExternalReactServerMessage, externalReactServerMessage } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maintains current behavior if the new ssr.resolve.conditions field is not set - backwards compatible.