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

fix(optimizer): keep NODE_ENV as-is when keepProcessEnv is true #18899

Merged
merged 4 commits into from
Dec 16, 2024
Merged

fix(optimizer): keep NODE_ENV as-is when keepProcessEnv is true #18899

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00d9992
fix(optimizer): keep NODE_ENV as-is when keepProcessEnv is `true`
sapphi-red Dec 6, 2024
ee24256
fix: define should be included in hash calc when keepProcessEnv is false
sapphi-red Dec 9, 2024
a8213b4
chore: rename variable
sapphi-red Dec 9, 2024
adcb59e
chore: remove unneeded mapping
sapphi-red Dec 9, 2024
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
12 changes: 8 additions & 4 deletions packages/vite/src/node/optimizer/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,9 +775,11 @@ async function prepareEsbuildOptimizerRun(
if (optimizerContext.cancelled) return { context: undefined, idToExports }

const define = {
'process.env.NODE_ENV': JSON.stringify(
process.env.NODE_ENV || environment.config.mode,
),
'process.env.NODE_ENV': environment.config.keepProcessEnv
? // define process.env.NODE_ENV even for keepProcessEnv === true
// as esbuild will replace it automatically when `platform` is `'browser'`
'process.env.NODE_ENV'
Comment on lines +778 to +781
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'process.env.NODE_ENV': environment.config.keepProcessEnv
? // define process.env.NODE_ENV even for keepProcessEnv === true
// as esbuild will replace it automatically when `platform` is `'browser'`
'process.env.NODE_ENV'
'process.env.NODE_ENV': environment.config.keepProcessEnv
? 'process.env.NODE_ENV'

is the comment really helpful? 🤔

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 it isn't obvious that we cannot write it like below without a comment.

const define = environment.config.keepProcessEnv
    ? {}
    : {
        'process.env.NODE_ENV': JSON.stringify(
          process.env.NODE_ENV || environment.config.mode,
        ),
      }

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, yeah that's not obvious 😅

I actually didn't understand this before, I thought that your comment was practically saying that process.env.NODE_ENV can be defined even for keepProcessEnv === true (since esbuild just replaces it) and not that it needs to

: JSON.stringify(process.env.NODE_ENV || environment.config.mode),
}

const platform =
Expand Down Expand Up @@ -1210,7 +1212,9 @@ function getConfigHash(environment: Environment): string {
const { optimizeDeps } = config
const content = JSON.stringify(
{
mode: process.env.NODE_ENV || config.mode,
define: config.keepProcessEnv
sapphi-red marked this conversation as resolved.
Show resolved Hide resolved
? process.env.NODE_ENV || config.mode
: null,
root: config.root,
resolve: config.resolve,
assetsInclude: config.assetsInclude,
Expand Down
19 changes: 19 additions & 0 deletions playground/environment-react-ssr/__tests__/environment-react-ssr.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,25 @@ describe.runIf(!isBuild)('pre-bundling', () => {
expect(metaJson.optimized['react/jsx-dev-runtime']).toBeTruthy()

expect(metaJson.optimized['react-dom/client']).toBeFalsy()

// process.env.NODE_ENV should be kept as keepProcessEnv is true
const depsFiles = fs
.readdirSync(path.resolve(testDir, 'node_modules/.vite/deps_ssr'), {
withFileTypes: true,
})
.filter((file) => file.isFile() && file.name.endsWith('.js'))
.map((file) => path.join(file.parentPath, file.name))
const depsFileHasProcessEnvNodeEnvLeft = depsFiles
.filter((file) =>
fs.readFileSync(file, 'utf-8').includes('process.env.NODE_ENV'),
)
.map((file) =>
path.relative(
path.resolve(testDir, 'node_modules/.vite/deps_ssr'),
file,
),
sapphi-red marked this conversation as resolved.
Show resolved Hide resolved
)
expect(depsFileHasProcessEnvNodeEnvLeft.length).toBeGreaterThan(0)
sapphi-red marked this conversation as resolved.
Show resolved Hide resolved
})

test('deps reload', async () => {
Expand Down
Loading