vite:define plugin will replace module or path-name keyword in defines

[genffy]

Describe the bug

if some lib, folder or file-name have some keyword defined in define property, and run vite build will casue [vite]: Rollup failed to resolve import error.

After checked source code and found some casue on follow picture.

Reproduction

open this link https://stackblitz.com/edit/vite-hxw2x7 and see Terminal output

System Info

System:
    OS: macOS 12.0.1
    CPU: (8) x64 Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
    Memory: 224.04 MB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.13.0 - /usr/local/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 8.1.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 95.0.4638.69
    Firefox: 78.15.0
    Safari: 15.1
  npmPackages:
    vite: workspace:* => 2.7.0-beta.3 

### Used Package Manager

pnpm

### Logs

_No response_

### Validations

- [X] Follow our [Code of Conduct](https://github.com/vitejs/vite/blob/main/CODE_OF_CONDUCT.md)
- [X] Read the [Contributing Guidelines](https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md).
- [X] Read the [docs](https://vitejs.dev/guide).
- [X] Check that there isn't [already an issue](https://github.com/vitejs/vite/issues) that reports the same bug to avoid creating a duplicate.
- [X] Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/vue-next instead.
- [X] Check that this is a concrete bug. For Q&A open a [GitHub Discussion](https://github.com/vitejs/vite/discussions) or join our [Discord Chat Server](https://chat.vitejs.dev/).
- [X] The provided reproduction is a [minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example) of the bug.

[genffy]

Nuxt3 case, nuxt/bridge#204

[patak-dev]

Vite docs specifically warn about using global here: https://vitejs.dev/config/#define, because it is common to want to use define in this way. But this isn't a Vite issue.

Because it's implemented as straightforward text replacements without any syntax analysis, we recommend using define for CONSTANTS only.
For example, process.env.FOO and __APP_VERSION__ are good fits. But process or global should not be put into this option. Variables can be shimmed or polyfilled instead.

@antfu @pi0 closing this issue, as it is already tracked in nuxt/bridge#204

This define rule should be removed, let's see what that was trying to workaround, maybe it isn't needed anymore:
https://github.com/nuxt/framework/blob/bf716f39783508e001a590c01956ac57937f6bd2/packages/vite/src/client.ts#L21

[pi0]

@patak-js Indeed makes sense to be fixed on the Nuxt side but how do you suggest to shim process and global for client modules?

BTW worth mentioning rollup's replace plugin uses escape to prevent some edge cases like this (https://github.com/rollup/plugins/blob/master/packages/replace/src/index.js#L44)

[Niputi]

shimming can be done in html file by writing window.global = window or window.global = globalThis

[Niputi]

issue should maybe be reopened again to improve define so it won't touch file names. I remember a similar issue, I'll try digging after it

[patak-dev]

About the process, and other node builtins in general, it is an issue for Vite in general. I think the ideal would be to invest time in getting something like https://github.com/snowpackjs/rollup-plugin-polyfill-node to work smoothly in Vite (this was working for Snowpack IIUC, so I think it should be an option). I think the community hasn't spent time in this because most of these cases end up being solved by switching an old dependency to a modern alternative, or by fixing the dep so they don't depend on node builtins.

About improving the regex, we have been merging small changes there. If there is a PR, we can check it with Evan.

@Niputi the paths are replaced in import statements, no? In that case, I think it is working as expected

[Niputi]

@patak-js it was this issue I remembered #4796 which seemed like define renames file names.

[pi0]

Thanks for the pointers @patak-js @Niputi. For now, I decided to remove global replacement until finding better replacement (nuxt/framework#1835)

shimming can be done in html file by writing window.global = window or window.global = globalThis

This seems a good idea. I would polyfill using globalThis.global = globalThis which works similarly but also in workers (envs that are neither Browser nor Node.js).

About the process, and other node builtins in general, it is an issue for Vite in general. I think the ideal would be to invest time in getting something like https://github.com/snowpackjs/rollup-plugin-polyfill-node to work smoothly in Vite (this was working for Snowpack IIUC, so I think it should be an option). I think the community hasn't spent time in this because most of these cases end up being solved by switching an old dependency to a modern alternative, or by fixing the dep so they don't depend on node builtins.

I would be more than happy to help with this. With Nuxt 3 and Nitro, we had to create a Node.js shimming framework unjs/unenv. It basically returns a framework agnostic preset of shims and polyfills. It already covers common use cases but I tend to increase Node.js coverage including fs support (Unimplemented shims are deep-mocked using mock/proxy .

Before starting unenv, I tried rollup-plugin-polyfill-node which is great but assuming to polyfill "Node for Browser" and based on browserify implementations which was not convincing at least for Nuxt since we needed Worker support as well.

We plan to use unenv for Nuxt 3 with vite soon (nuxt/nuxt#12786) will let you know how it goes and (probably adding process and global shims in the same PR)

---

Regarding this issue, I let you judge worth improving on vite's side or not. The behavior seems expected for replacement -- however vite could cover more edge cases and bade replacements out of the box on the other hand, it can add more codebase complexity.

[patak-dev]

https://github.com/unjs/unenv looks great @pi0, and it would be great for Vite if there was an easy way to drop it in as a Vite plugin or something similar so we could recommend if users are facing issues with their deps. I still think that long term we should keep pushing for the ecosystem to fix this in each package, but this could alleviate a lot of temporal pain.

[genffy]

@patak-js , As @pi0 mentioned, I recommend vite to add dev mode warning for define’s key that contain some private or inside keywords in JavaScript. That is if no plan to make this replacement smoothly.