fix: avoid replacing defines and NODE_ENV in optimized deps (fix #8593)

[patak-dev]

Description

When we use define for build optimized deps and there is CJS interop, the defined keys end up being used as part of the interop. In the define playground, it looks like:

// <define:__OBJ__>
var init_define_OBJ = __esm({
  "<define:__OBJ__>"() {
  }
});

// <define:__STRINGIFIED_OBJ__>
var foo, define_STRINGIFIED_OBJ_default;
var init_define_STRINGIFIED_OBJ = __esm({
  "<define:__STRINGIFIED_OBJ__>"() {
    foo = true;
    define_STRINGIFIED_OBJ_default = { foo };
  }
});

So if we don't skip these files in the define plugin, then the keys are re-replaced and there is an error.

We have two ways to fix this:

1. the first commit

• we don't use define in esbuild optimization during build
• we let the define plugin process this

cons:

• there is a failing test, I don't understand why

2. the second commit

• we let define in esbuild untouched
• we skip the optimized deps in the define plugin

pro:

• dev and prod are more similar

cons:

• the define keys appear in the optimize dep, and also in the final output?
• the failing test is gone

Pushing so we can continue the discussion here

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[sapphi-red]

esbuild's platform option's default is browser.
I found that when platform is browser, esbuild defines process.env.NODE_ENV automatically.
https://github.com/evanw/esbuild/blob/e03d4732bb9c5443dbfbb2f9e8cbcd5de2572632/pkg/api/api_impl.go#L578-L602

This might be the reason of the failing test.

[sapphi-red]

dd68152#diff-456b3c61f2f19b160b6a5016cb9a180500c4e3b3a9a41a192ca24670637e7706R475-R487
If I changed this part to

  const define: Record<string, string> = {
    'process.env.NODE_ENV': JSON.stringify(
      process.env.NODE_ENV || config.mode
    )
  }
  if (!isBuild) {
    // We only use define for dev optimized deps. During build we let the define keys
    // unchanged as the define plugin will process them
    for (const key in config.define) {
      const value = config.define[key]
      define[key] = typeof value === 'string' ? value : JSON.stringify(value)
    }
  }

the tests passed.

[sapphi-red]

BTW What is the purpose of using esbuild's define during dev optimization? I was curious since define plugin does not run during dev.

[patak-dev]

BTW What is the purpose of using esbuild's define during dev optimization? I was curious since define plugin does not run during dev.

The define plugins don't run during dev as an optimization, and we have the defines as global variables in the client env directly:

vite/packages/vite/src/client/env.ts

Line 17 in 7157b15

const defines = __DEFINES__

So, during the regular dev I agree that they aren't needed since we have the globals. The defines where added in this commit:

• fix(ssr): respect user defines for ssr

But I don't understand why we are doing it, before optimized deps weren't used during SSR dev, and now the define plugin still does a replacement:

vite/packages/vite/src/node/plugins/define.ts

Line 119 in 7157b15

if (ssr && !isBuild) {

. It is a simple replacement, that I think we can do because for our defines we explicitly tell the user that it should be a safe name. We don't replace node env.

Maybe we should directly remove the defines during esbuild optimization?
And for the process.env replacements, we should also remove that in SSR deps optimization.

[patak-dev]

esbuild's platform option's default is browser. I found that when platform is browser, esbuild defines process.env.NODE_ENV automatically. evanw/esbuild@e03d473/pkg/api/api_impl.go#L578-L602

This might be the reason of the failing test.

Thanks for this pointer, this was indeed the problem. It also showed that we were missing changing the platform to 'node' for SSR + ssr.target !== 'webworker'.

In lib mode, we are keeping process.node.env untouched, so to be able to keep doing the same I added a define of process.env.NODE_ENV to __vite_process_env_NODE_ENV that is later replaced by the define plugin (this is only done during build).
The ideal would be for esbuild to allow us to avoid the automatic replacement of process.node.env in browser mode but I couldn't find a way to do that.

The user defines have now been removed from optimize deps, I think they aren't actually needed at the optimize deps level.

[sapphi-red]

The ideal would be for esbuild to allow us to avoid the automatic replacement of process.node.env in browser mode but I couldn't find a way to do that.

Because we are handling package resolution and externalization, maybe we could use platform: 'neutral' for both non-SSR and SSR. IIUC all things written in docs of platform option is covered by esbuildDepPlugin in Vite.

The user defines have now been removed from optimize deps, I think they aren't actually needed at the optimize deps level.

I think so, too. This will fix #8526 (It was closed but I was wondering if I could do a better handling).

[patak-dev]

The ideal would be for esbuild to allow us to avoid the automatic replacement of process.node.env in browser mode but I couldn't find a way to do that.

Because we are handling package resolution and externalization, maybe we could use platform: 'neutral' for both non-SSR and SSR. IIUC all things written in docs of platform option is covered by esbuildDepPlugin in Vite.

Agreed, I thought of this but wanted to avoid doing further changes to the way we optimize during dev but I think you are right here, let test this now. This is another piece where we are removing differences between dev and build. And there is no perf regression, esbuild is just bundling and not applying tree-shaking at this point.

[bluwy]

Nice! @sapphi-red brought up a good point about define in optimized deps in define, and I think it's safe that we don't do that too (for reasons above).

I think this should be good

[sapphi-red]

LGTM 🚀

[Chen-jj]

BTW What is the purpose of using esbuild's define during dev optimization? I was curious since define plugin does not run during dev.

The define plugins don't run during dev as an optimization, and we have the defines as global variables in the client env directly:

vite/packages/vite/src/client/env.ts

Line 17 in 7157b15

const defines = __DEFINES__

So, during the regular dev I agree that they aren't needed since we have the globals. The defines where added in this commit:

• fix(ssr): respect user defines for ssr

But I don't understand why we are doing it, before optimized deps weren't used during SSR dev, and now the define plugin still does a replacement:

vite/packages/vite/src/node/plugins/define.ts

Line 119 in 7157b15

if (ssr && !isBuild) {

. It is a simple replacement, that I think we can do because for our defines we explicitly tell the user that it should be a safe name. We don't replace node env.
Maybe we should directly remove the defines during esbuild optimization? And for the process.env replacements, we should also remove that in SSR deps optimization.

@patak-dev @sapphi-red @bluwy Removing the esbuild defines during esbuild optimization bring problems at dynamic imports case:

if (FRAMEWORK === 'preact') {
  const options = require('preact').options
 // ...
}

For example, when user are using react, we define FRAMEWORK: react here.

In Vite2, vite' define options pass to esbuild' define too.esbuild will had dead code eliminated and everything ok:

// Vite2 output
if (false) {
  const options = null.options
}

However, after removing esbuild' define. Vite only define FRAMEWORK at runtime.During deps scanning stage, esbuild will try to bundle preact which the user had not installed because they are using react.So esbuild throws an error that preact not found.

// Vite3 output
if (FRAMEWORK === 'preact') {
  const options = require('preact').options
 // ...
}

Besides, Webpack's output:

if (false) {}

And maybe #5676 related.

[bluwy]

@Chen-jj Thanks for the explanation! I think that makes sense though it feels a bit off to have custom user defines intercept dependency code for treeshaking. Nonetheless I think it wouldn't hurt to re-add this, can you create a feature request so we can keep track of this?

[Chen-jj]

@Chen-jj Thanks for the explanation! I think that makes sense though it feels a bit off to have custom user defines intercept dependency code for treeshaking. Nonetheless I think it wouldn't hurt to re-add this, can you create a feature request so we can keep track of this?

Sure :), #9527