fix: use Function instead of eval to dynamically import config files

[SamVerschueren]

Description

Hi Everyone 👋 . I'm a software engineer at StackBlitz. When I tested out the Vite example for Svelte through https://vite.new/svelte I noticed that something broke. After some investigation I could track it down to the eval() dynamic import workaround. Currently dynamic imports in eval() don't work yet, we're looking at implementing them but can't give any estimate when that would be the case. However, dynamic imports do work in new Function() calls and also make the code a bit cleaner and reusable I guess, but that might be just a personal opinion.

After making these changes directly in node_modules/vite, it works on StackBlitz as well.

If this would be merged, this PR #5197 should make use of the dynamicImport method in utils.ts as well.

Additional context

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[patak-dev]

Hey @SamVerschueren! Great to see the StackBlitz team digging into Vite's codebase.

The fix looks good to me. I just re-run the jobs, there was a segmentation fault without a clear error. Let's see if it is just a glitch

[patak-dev]

Still failing, is the test suite working for you locally @SamVerschueren ?

[patak-dev]

@aleclarson is telling me that this may be a GH glitch

[SamVerschueren]

It might be something with the test. Let me dig a bit deeper.

[SamVerschueren]

I found a fix, but I'm not sure if I'm a fan of it. The thing is that Jest seem to enforce the use of NODE_OPTIONS=--experimental-vm-modules (https://jestjs.io/docs/ecmascript-modules).

Without that flag, Jest seems to choke on the dynamic import. But enabling that flag for all tests just feels a bit weird.

What do you guys think?

[SamVerschueren]

The tests still cause a segfault on local as well. If I remove the Jest flag --runInBand it works. It's definitely caused by the additional test, because if I disable the test, it works as well.

[dominikg]

i'm seeing this error message, even after changing the fixture from vite.config.mjs to vite.config.cjs.

dominikg@mb14:~/develop/vite$ pnpm test-serve -- --maxWorkers=4 --testPathPattern=utils.spec.ts

> vite-monorepo@ test-serve /home/dominikg/develop/vite
> jest "--maxWorkers=4" "--testPathPattern=utils.spec.ts"

 FAIL  packages/vite/src/node/__tests__/utils.spec.ts
  ✓ path with multiple spaces (1 ms)
  ✓ path with multiple % characters
  ✓ path with %25
  ✓ path with unicode
  ✓ path with unicode, space, and % (1 ms)
  ✕ dynamic import (1 ms)

  ● dynamic import

    You need to run with a version of node that supports ES Modules in the VM API. See https://jestjs.io/docs/ecmascript-modules

      at invariant (node_modules/.pnpm/jest-runtime@27.2.4/node_modules/jest-runtime/build/index.js:2417:11)
      at eval (eval at Object.<anonymous> (packages/vite/src/node/utils.ts:577:30), <anonymous>:3:1)
      at Object.<anonymous> (packages/vite/src/node/__tests__/utils.spec.ts:45:24)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 5 passed, 6 total
Snapshots:   0 total
Time:        3.007 s

Which indicates that your addition of --experimental-vm-modules could work. But i don't like it at all. vite's tests have been largely written before jest supported esm and all the e2e tests still use cjs.
Enabling that flag should come with converting the vite tests to esm, which would be a massive undertaking outside of the scope of this PR.

I'd rather see if we can find a way to test this via e2e tests that don't cause this problem. eg. playground/cli testsuite does load a vite.config.js with the code of this PR, so it could count as sort of a 'transitive' testcase.

One thing i am a bit worried about is the dynamic cache buster query param that isn't part of the dynamicImport function itself, if all usage in vite is requiring this cache buster, it could be inlined to avoid people forgetting it.

[SamVerschueren]

I have to remove the test because it feels like there's nothing I can do myself. I used segfault-handler to dump a crash log and this is what I get.

PID 38882 received SIGSEGV for address: 0x0
0   segfault-handler.node               0x00000001046d808a _ZL16segfault_handleriP9__siginfoPv + 298
1   libsystem_platform.dylib            0x00007fff20385d7d _sigtramp + 29
2   ???                                 0x0000000107034c90 0x0 + 4412624016
3   node                                0x000000010033d15a _ZN2v88internal7Isolate38RunHostImportModuleDynamicallyCallbackENS0_6HandleINS0_6ScriptEEENS2_INS0_6ObjectEEE + 138
4   node                                0x00000001006f7574 _ZN2v88internal25Runtime_DynamicImportCallEiPmPNS0_7IsolateE + 340
5   node                                0x0000000100a71a94 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit + 52
6   node                                0x0000000100aeea2e Builtins_CallRuntimeHandler + 78
7   node                                0x0000000100a0ac82 Builtins_InterpreterEntryTrampoline + 194
8   node                                0x0000000100a0ac82 Builtins_InterpreterEntryTrampoline + 194
[1]    38875 segmentation fault  NODE_OPTIONS=--experimental-vm-modules npx pnpm run test-serve -- --runInBand

This does not happen if I remove the --runInBand flag or if I only run the utils spec file.

[dominikg]

Note, in vite-plugin-svelte we do use a crude try/catch solution that falls back to require in case dynamic import fails, so that it works even when dynamic import isn't supported. https://github.com/sveltejs/vite-plugin-svelte/blob/4f24b678d930292b62ad69f141580469e48f6224/packages/vite-plugin-svelte/src/utils/load-svelte-config.ts#L28

I'm not sure this would be the right move for vite though, it's more or less just present for legacy cjs projects. (sometimes i wish they'd went for a big breaking change instead of this dual-system compatibility nightmare.)

[SamVerschueren]

I agree, I'd rather not introduce this flag either. If Vite had e2e tests with config files, I think we're fine because then it's covered at a higher level.

At first I had the cache buster added inside the new Function directly but wasn't sure if it was a good thing to always add it. But I can definitely move it if that's wanted.

[SamVerschueren]

Also, this PR #5197 for instance does use a dynamic import as well, but without cache buster. I think that was the reason I didn't add it for every import. Maybe that PR also needs it though, in that case it might make sense to add it for everything.

[aleclarson]

At first I had the cache buster added inside the new Function directly but wasn't sure if it was a good thing to always add it. But I can definitely move it if that's wanted.

No thanks. We need to allow caching if ssrModuleLoader is to use this function.

[dominikg]

I tried to verify that the current tests fail if dynamicImport throws an error but it didn't turn out as i expected. As said above the e2e tests are cjs, so there is no "type": "module" in their package.json and the code path with dynamicImport isn't taken.

a copy of packages/playground/cli as packages/playground/cli-module with "type":"module" in it's package.json would however throw on any errors happening then.

[benmccann]

@SamVerschueren do you have a copy of the test that caused the segfault? I'd like to look at it, but it looks like the commits were squashed, so I'm not sure how to reproduce