Skip to content

refactor: simplify build optimizer node_env handling#14829

Merged
bluwy merged 1 commit into
mainfrom
optimizer-node-env-handling
Nov 1, 2023
Merged

refactor: simplify build optimizer node_env handling#14829
bluwy merged 1 commit into
mainfrom
optimizer-node-env-handling

Conversation

@bluwy
Copy link
Copy Markdown
Member

@bluwy bluwy commented Oct 31, 2023

Description

We can workaround the change in optimizer/index.ts with define: { 'process.env.NODE_ENV': 'process.env.NODE_ENV' } instead. (esbuild example)

Also I'm not sure if the check was wrong. The comment mentioned lib builds but we were checking generic builds. I changed the condition to check lib builds instead (same as define.ts)

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Oct 31, 2023
@bolt-new-by-stackblitz
Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Copy Markdown
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

I read through #8606 and #8714. It looks good to me.
Do we have a test for preserving process.env in a optimizedDep somewhere?

@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented Nov 1, 2023

Hmm looks like we don't. All tests pass if I set it like this:

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

I suppose we don't have a lot of test for the build optimizer + lib mode. Let me try to add a test.

Copy link
Copy Markdown
Member

@patak-cat patak-cat left a comment

Choose a reason for hiding this comment

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

It seems the condition was wrong, this looks good to me too

@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented Nov 1, 2023

I tried to add a test (in main for testing) and it's not quite working for the build optimizer with lib mode I think. Getting strange errors:

trace
vite v5.0.0-beta.14 building for production...
Unexpected early exit. This happens when Promises returned by plugins cannot resolve. Unfinished hook action(s) on exit:
(vite:build-import-analysis) transform "/Users/bjorn/Work/oss/vite/playground/lib/src/build-optimize.js"
(commonjs--resolver) resolveId "@vitejs/test-pkg-with-process-node-env" "/Users/bjorn/Work/oss/vite/playground/lib/src/build-optimize.js"
(vite:optimized-deps-build) load "/Users/bjorn/Work/oss/vite/playground/lib/node_modules/.vite-build-optimize/deps_build-1b690276/@vitejs_test-pkg-with-process-node-env.js"
error during build:
Error: Unexpected early exit. This happens when Promises returned by plugins cannot resolve. Unfinished hook action(s) on exit:
(vite:build-import-analysis) transform "/Users/bjorn/Work/oss/vite/playground/lib/src/build-optimize.js"
(commonjs--resolver) resolveId "@vitejs/test-pkg-with-process-node-env" "/Users/bjorn/Work/oss/vite/playground/lib/src/build-optimize.js"
(vite:optimized-deps-build) load "/Users/bjorn/Work/oss/vite/playground/lib/node_modules/.vite-build-optimize/deps_build-1b690276/@vitejs_test-pkg-with-process-node-env.js"
    at process.handleBeforeExit (file:///Users/bjorn/Work/oss/vite/node_modules/.pnpm/rollup@4.2.0/node_modules/rollup/dist/es/shared/node-entry.js:18801:28)
    at Object.onceWrapper (node:events:628:26)
    at process.emit (node:events:513:28)
    at process.emit (/Users/bjorn/Work/oss/vite/node_modules/.pnpm/source-map-support@0.5.21/node_modules/source-map-support/source-map-support.js:516:21)
 ELIFECYCLE  Command failed with exit code 1.

I'll merge it in for now then.

@bluwy bluwy merged commit 275907b into main Nov 1, 2023
@bluwy bluwy deleted the optimizer-node-env-handling branch November 1, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p2-nice-to-have Not breaking anything but nice to have (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants