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(plugin-legacy): prevent global process.env.NODE_ENV mutation #9741

Merged
merged 4 commits into from
Aug 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions packages/plugin-legacy/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ function viteLegacyPlugin(options: Options = {}): Plugin[] {
modernPolyfills
)
await buildPolyfillChunk(
config.mode,
modernPolyfills,
bundle,
facadeToModernPolyfillMap,
Expand Down Expand Up @@ -237,6 +238,7 @@ function viteLegacyPlugin(options: Options = {}): Plugin[] {
)

await buildPolyfillChunk(
config.mode,
legacyPolyfills,
bundle,
facadeToLegacyPolyfillMap,
Expand Down Expand Up @@ -615,6 +617,7 @@ function createBabelPresetEnvOptions(
}

async function buildPolyfillChunk(
mode: string,
imports: Set<string>,
bundle: OutputBundle,
facadeToChunkMap: Map<string, string>,
Expand All @@ -626,6 +629,7 @@ async function buildPolyfillChunk(
let { minify, assetsDir } = buildOptions
minify = minify ? 'terser' : false
const res = await build({
mode,
// so that everything is resolved from here
root: path.dirname(fileURLToPath(import.meta.url)),
configFile: false,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { describe, expect, test } from 'vitest'
import { port } from './serve'
import { isBuild, page } from '~utils'

const url = `http://localhost:${port}`

describe.runIf(isBuild)('client-legacy-ssr-sequential-builds', () => {
test('should work', async () => {
await page.goto(url)
expect(await page.textContent('#app')).toMatch('Hello')
})

test('import.meta.env.MODE', async () => {
// SSR build is always modern
expect(await page.textContent('#mode')).toMatch('test')
})
})
68 changes: 68 additions & 0 deletions playground/legacy/__tests__/client-and-ssr/serve.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// this is automatically detected by playground/vitestSetup.ts and will replace
// the default e2e test serve behavior
import path from 'node:path'
import { ports, rootDir } from '~utils'

export const port = ports['legacy/client-and-ssr']

export async function serve(): Promise<{ close(): Promise<void> }> {
const { build } = await import('vite')

// In a CLI app it is possible that you may run `build` several times one after another
// For example, you may want to override an option specifically for the SSR build
// And you may have a CLI app built for that purpose to make a more concise API
// An unexpected behaviour is for the plugin-legacy to override the process.env.NODE_ENV value
// And any build after the first client build that called plugin-legacy will misbehave and
// build with process.env.NODE_ENV=production, rather than your CLI's env: NODE_ENV=myWhateverEnv my-cli-app build
// The issue is with plugin-legacy's index.ts file not explicitly passing mode: process.env.NODE_ENV to vite's build function
// This causes vite to call resolveConfig with defaultMode = 'production' and mutate process.env.NODE_ENV to 'production'

await build({
mode: process.env.NODE_ENV,
root: rootDir,
logLevel: 'silent',
build: {
target: 'esnext',
outDir: 'dist/client'
}
})

await build({
mode: process.env.NODE_ENV,
root: rootDir,
logLevel: 'silent',
build: {
target: 'esnext',
ssr: 'entry-server-sequential.js',
outDir: 'dist/server'
}
})

const { default: express } = await import('express')
const app = express()

app.use('/', async (_req, res) => {
const { render } = await import(
path.resolve(rootDir, './dist/server/entry-server-sequential.mjs')
)
const html = await render()
res.status(200).set({ 'Content-Type': 'text/html' }).end(html)
})

return new Promise((resolve, reject) => {
try {
const server = app.listen(port, () => {
resolve({
// for test teardown
async close() {
await new Promise((resolve) => {
server.close(resolve)
})
}
})
})
} catch (e) {
reject(e)
}
})
}
7 changes: 7 additions & 0 deletions playground/legacy/entry-server-sequential.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// This counts as 'server-side' rendering, yes?
export async function render() {
return /* html */ `
<div id="app">Hello</div>
<div id="mode">${import.meta.env.MODE}</div>
`
}
1 change: 1 addition & 0 deletions playground/test-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export const ports = {
'legacy/ssr': 9520,
lib: 9521,
'optimize-missing-deps': 9522,
'legacy/client-and-ssr': 9523,
'ssr-deps': 9600,
'ssr-html': 9601,
'ssr-pug': 9602,
Expand Down