fix(ssr): hoist export to handle cyclic import better by hi-ogawa · Pull Request #18983 · vitejs/vite

hi-ogawa · 2024-12-17T08:40:52Z

Description

closes Vite SSR Imports not loading classes correctly - constructors not loaded #14048

Currently Object.defineProperty(__vite_ssr_exports__, ...) is not hoisted and this is different from what bundlers normally do (see my notes in https://github.com/hi-ogawa/reproductions/tree/main/cyclic-import). By defining __vite_ssr_exports__ property early before await __vite_ssr_import__(...) calls, it fixes certain cyclic import scenarios.

For export * from "...", bundlers statically analyze the export names and hoist them too, but this is not possible for Vite SSR, so __vite_ssr_exportAll__ is kept as is.

One potential issue with this change is that this can technically break a "working" code with cyclic import which were previously forgiven. This was actually the case of our own unit test. See #18983 (comment) for details.

todo

fix broken unit-tests with bad cyclic import entry
- go with setupFiles: ['./packages/vite/src/node/index.ts'] trick
compare with rollup output
- https://github.com/hi-ogawa/reproductions/tree/main/vite-18983-rollup-cyclic-import
- should work similar to this PR
review ecosystem failure
- ~~vitest: broken source map?~~ (the issue is on Vitest side test: debug Vite PR 18983 vitest-dev/vitest#7096)
- vitest: broken coverage Missing function coverage when transform prepends code right before the function vitest-dev/vitest#7130
  - ~~this could be a blocker. we probably need some judgement from Ari.~~
  - ~~this doesn't happen if we ensure ;\n when hoisting, but that would entirely diminish the effort of fix(ssrTransform): preserve line offset when transforming imports #19004~~
  - Now giving up line preservation to ensure Vitest coverage won't get easily broken. The initial motivation of line-preservation was for broken debugging on Vitest, but that is separately fixed on Vitest side by fix(vite-node): fix source map of inlined node_modules vitest-dev/vitest#7557

package.json

playground/hmr-ssr/__tests__/hmr-ssr.spec.ts

hi-ogawa · 2024-12-17T09:20:36Z

/ecosystem-ci run

pkg-pr-new · 2024-12-17T09:21:50Z

Open in Stackblitz

npm i https://pkg.pr.new/vite@18983

commit: 7e7b660

vite-ecosystem-ci · 2024-12-17T09:54:35Z

📝 Ran ecosystem CI on 3cc86da: Open

suite	result	latest scheduled
histoire	❌ failure	❌ failure
laravel	❌ failure	✅ success
marko	❌ failure	✅ success
nuxt	❌ failure	✅ success
redwoodjs	❌ failure	❌ failure
remix	❌ failure	❌ failure
vike	❌ failure	❌ failure
vite-plugin-svelte	❌ failure	❌ failure
vitest	❌ failure	✅ success
waku	❌ failure	❌ failure

✅ analogjs, astro, ladle, previewjs, quasar, qwik, rakkas, storybook, sveltekit, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

hi-ogawa · 2024-12-17T10:02:12Z

vitest.config.ts

+    // TODO:
+    // importing non entry file can be broken due to cyclic import e.g.
+    //   pnpm exec tsx packages/vite/src/node/server/index.ts
+    setupFiles: ['./packages/vite/src/node/index.ts'],


Interestingly, export hoisting can make cyclic import handling stricter as seen by:

$ pnpm test-unit packages/vite/src/node/ssr/runtime/__tests__/server-runtime.spec.ts FAIL packages/vite/src/node/ssr/runtime/__tests__/server-runtime.spec.ts [ packages/vite/src/node/ssr/runtime/__tests__/server-runtime.spec.ts ] ReferenceError: Cannot access 'serverConfigDefaults' before initialization ❯ Module.get [as serverConfigDefaults] packages/vite/src/node/server/index.ts:4:116

but this case might be legitimate since this is the same error as tsx:

$ pnpm exec tsx packages/vite/src/node/server/index.ts /home/hiroshi/code/others/vite/packages/vite/src/node/config.ts:648 server: serverConfigDefaults, ^ ReferenceError: Cannot access 'serverConfigDefaults' before initialization

I'd imagine previously these were undefined (still wrong but no hard error) since __vite_ssr_import_x_.serverConfigDefaults getter was not defined. But now this hits uninitialized const serverConfigDefaults access due to getter.

Just temporarily, I added this silly setupFiles, so that modules will be initialized in a known safe order.

hi-ogawa · 2024-12-18T02:15:37Z

playground/hmr-ssr/__tests__/hmr-ssr.spec.ts

+    // it throws a same error as browser case,
+    // but it doesn't auto reload and it calls `hot.accept` called with `undefined`
+    await untilUpdated(() => el(), '')


It turns out the error now became same as playground/hmr but module runner doesn't auto-reload, so the existing test failed.

ReferenceError: Cannot access 'c' before initialization

I adjust this test case for now to make it pass.

hi-ogawa · 2024-12-25T05:39:24Z

/ecosystem-ci run

vite-ecosystem-ci · 2024-12-25T06:12:46Z

📝 Ran ecosystem CI on 83a7a1f: Open

suite	result	latest scheduled
histoire	❌ failure	❌ failure
ladle	❌ failure	❌ failure
nuxt	✅ success	❌ failure
redwoodjs	❌ failure	❌ failure
remix	❌ failure	❌ failure
vite-plugin-svelte	❌ failure	❌ failure
vitest	❌ failure	❌ failure
waku	❌ failure	❌ failure

✅ analogjs, astro, laravel, marko, previewjs, quasar, qwik, rakkas, storybook, sveltekit, unocss, vike, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

vite-ecosystem-ci · 2024-12-25T07:07:21Z

📝 Ran ecosystem CI on 83a7a1f: Open

suite	result	latest scheduled
histoire	❌ failure	❌ failure
ladle	❌ failure	❌ failure
nuxt	✅ success	❌ failure
redwoodjs	❌ failure	❌ failure
remix	❌ failure	❌ failure
vite-plugin-svelte	❌ failure	❌ failure
vitest	❌ failure	❌ failure
waku	❌ failure	❌ failure

✅ analogjs, astro, laravel, marko, previewjs, quasar, qwik, rakkas, storybook, sveltekit, unocss, vike, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

hi-ogawa · 2025-03-07T01:46:36Z

/ecosystem-ci run

vite-ecosystem-ci · 2025-03-07T02:03:27Z

📝 Ran ecosystem CI on 7e7b660: Open

suite	result	latest scheduled
sveltekit	❌ failure	✅ success
vite-plugin-cloudflare	❌ failure	✅ success
astro	❌ failure	✅ success
vitest	❌ failure	✅ success
vite-plugin-svelte	✅ success	❌ failure

✅ ladle, histoire, previewjs, nuxt, analogjs, marko, qwik, react-router, storybook, vite-plugin-react, vite-environment-examples, laravel, quasar, vite-plugin-pwa, vite-plugin-react-swc, unocss, vite-setup-catalogue, vite-plugin-vue, vitepress, waku, vike, vuepress, rakkas

vite-ecosystem-ci · 2025-03-07T02:18:55Z

📝 Ran ecosystem CI on 7e7b660: Open

suite	result	latest scheduled
vite-plugin-svelte	✅ success	❌ failure
sveltekit	❌ failure	✅ success
vitest	❌ failure	✅ success
vite-plugin-cloudflare	❌ failure	✅ success

✅ histoire, quasar, react-router, vite-environment-examples, previewjs, qwik, storybook, analogjs, vike, nuxt, vite-plugin-react-swc, rakkas, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-vue, ladle, laravel, marko, astro, waku, vite-setup-catalogue, vitepress, vuepress

hi-ogawa · 2025-03-07T03:04:22Z

This looks good on Vitest side as confirmed in vitest-dev/vitest#7621

Aligning with vitejs/vite#18983 I manually verified the test case output matches with the ones in Vite repo. ~I add a new TODO about "export default" getter, but this difference existed already. I plan to fix this separately.~ (EDIT: It turned out Vite's transform is the wrong one vitejs/vite#19834.)

Co-authored-by: sapphi-red <[email protected]>

hi-ogawa added 3 commits December 17, 2024 17:25

fix(ssr): hoist export to handle cyclic import better

5f8c6fd

chore: remove override temporarily

b537976

wip: forget this one too for now

3cc86da

hi-ogawa force-pushed the fix-ssr-transform-hoist-export branch from ff8cf4b to 3cc86da Compare December 17, 2024 09:09

hi-ogawa commented Dec 17, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

playground/hmr-ssr/__tests__/hmr-ssr.spec.ts Outdated Show resolved Hide resolved

vite-ecosystem-ci bot added the trigger: preview label Dec 17, 2024

hi-ogawa added 2 commits December 17, 2024 18:39

wip: workaround cyclic import

ed79e4d

chore: unused

95f47eb

hi-ogawa commented Dec 17, 2024

View reviewed changes

test: fix hmr-ssr

2245aaa

hi-ogawa force-pushed the fix-ssr-transform-hoist-export branch from 4e35256 to 2245aaa Compare December 18, 2024 02:14

hi-ogawa commented Dec 18, 2024

View reviewed changes

hi-ogawa added 3 commits December 18, 2024 11:19

Merge branch 'main' into fix-ssr-transform-hoist-export

a8c1d12

test: fix importing broken entries

c2f7d75

test: tweak

96f123a

This was referenced Dec 18, 2024

test: debug Vite PR 18983 vitest-dev/vitest#7096

Closed

Feedback on Rolldown Dev (hmr-poc branch) rolldown/rolldown#2866

Closed

hi-ogawa added 5 commits December 25, 2024 14:05

Merge branch 'main' into fix-ssr-transform-hoist-export

d5b2333

fix: remove new line

d252b5c

test: update

7904808

chore: comment

45e74ac

test: test invalid cyclic

83a7a1f

hi-ogawa added 2 commits December 25, 2024 15:54

chore: comment

af7de84

chore: comment

b0b75eb

This was referenced Feb 27, 2025

Vite SSR doesn't overwrite * re-export by named export #19525

Closed

fix(ssr): named export should overwrite export all #19534

Merged

hi-ogawa added 3 commits March 7, 2025 10:09

Merge branch 'main' into fix-ssr-transform-hoist-export

7d75d1f

fix: give up preserving lines

1d5110e

fix: tweak new lines

7e7b660

hi-ogawa mentioned this pull request Mar 7, 2025

chore: test Vite PR 18983 vitest-dev/vitest#7621

Closed

6 tasks

hi-ogawa marked this pull request as ready for review March 7, 2025 03:04

sapphi-red added this to the 6.3 milestone Mar 7, 2025

sapphi-red approved these changes Mar 7, 2025

View reviewed changes

hi-ogawa added feat: ssr and removed p2-to-be-discussed Enhancement under consideration (priority) labels Mar 19, 2025

sapphi-red merged commit 8c04c69 into vitejs:main Mar 26, 2025
27 of 28 checks passed

hi-ogawa deleted the fix-ssr-transform-hoist-export branch March 26, 2025 01:32

sheremet-va mentioned this pull request Apr 4, 2025

vite-node: race condition in exports resulting in empty modules vitest-dev/vitest#7797

Closed

6 tasks

This was referenced Apr 10, 2025

chore: use oxc module runner transform vitejs/rolldown-vite#85

Draft

fix(transformer/module_runner): hoist export oxc-project/oxc#10347

Merged

wrong execution order with re-export on Vite SSR #19839

Closed

hi-ogawa mentioned this pull request Apr 16, 2025

fix(vite-node): named export should overwrite export all vitest-dev/vitest#7846

Merged

6 tasks

jbaldassari mentioned this pull request Apr 22, 2025

Export overrides broken in tests with Vite 6.3.0+ #19915

Closed

7 tasks

hi-ogawa mentioned this pull request May 2, 2025

fix(ssr): handle uninitialized export access as undefined #19959

Merged

hi-ogawa mentioned this pull request May 23, 2025

SSR transformed named imported functions don't work with debuggers "step-into" #18325

Open

7 tasks

moonlitusun pushed a commit to moonlitusun/vite that referenced this pull request May 25, 2025

fix(ssr): hoist export to handle cyclic import better (vitejs#18983)

c0b4ca4

Co-authored-by: sapphi-red <[email protected]>

Uh oh!

Comments

Conversation

hi-ogawa commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

Uh oh!

hi-ogawa commented Dec 17, 2024

Uh oh!

pkg-pr-new bot commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vite-ecosystem-ci bot commented Dec 17, 2024

Uh oh!

hi-ogawa Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hi-ogawa Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hi-ogawa commented Dec 25, 2024

Uh oh!

vite-ecosystem-ci bot commented Dec 25, 2024

Uh oh!

vite-ecosystem-ci bot commented Dec 25, 2024

Uh oh!

hi-ogawa commented Mar 7, 2025

Uh oh!

vite-ecosystem-ci bot commented Mar 7, 2025

Uh oh!

vite-ecosystem-ci bot commented Mar 7, 2025

Uh oh!

hi-ogawa commented Mar 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hi-ogawa commented Dec 17, 2024 •

edited

Loading

pkg-pr-new bot commented Dec 17, 2024 •

edited

Loading

hi-ogawa Dec 17, 2024 •

edited

Loading

hi-ogawa Dec 18, 2024 •

edited

Loading