fix(ssr): crash on circular import

[Dunqing]

Description

An interesting bug, I stumbled upon ssrTransform changing the order of import/export. Yes! This problem is essentially caused by a change in the order of import/export.

But I'm not sure it's completely fixed.

Fixes: #14433
Fixes: #14010

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 PR Title 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.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[bluwy]

LGTM. Thanks for digging into this.

Looks like this makes a partial revert of #12274 + #12530. I think it makes sense that the "import" hoisting behaviour doesn't exactly apply to export {} and export *. I remember testing a lot of these edge cases, but I didn't hit this.

[rtsao]

LGTM! I think there is an unresolved nit regarding types of defineImport

[Dunqing]

I think this PR can be merged first

[bluwy]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
analogjs	✅ success
astro	✅ success
histoire	✅ success
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt	✅ success
nx	❌ failure
previewjs	✅ success
qwik	❌ failure
rakkas	✅ success
sveltekit	✅ success
unocss	✅ success
vike	✅ success
vite-plugin-pwa	✅ success
vite-plugin-react	✅ success
vite-plugin-react-pages	✅ success
vite-plugin-react-swc	✅ success
vite-plugin-svelte	✅ success
vite-plugin-vue	✅ success
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success

[bluwy]

Besides nx which seems to take too long to test, the rest seems to match main, so merging this would be fine by me. I find the still incorrect import/export execution order a little worrying however, but we could fix that later on.

[Dunqing]

I find the still incorrect import/export execution order a little worrying however, but we could fix that later on.

How did you find out about this? I want to do my part for this.

[bluwy]

In the import and export ordering test, I have a comment that explained the desired execution order of console.logs. With this PR's new snapshot, if you run the logs in your head you should get "mod foo", "bar1", "mod a", "mod b", "bar2".

I'm not exactly sure how to fix it but thanks for helping with this.

[sapphi-red]

Suggested change

	export function getB() {
	return '__B__'
	}
	export { A } from './a'
	export { A } from './a'
	export function getB() {
	return '__B__'
	}

If I change this file like above, the test fails with getB is not a function. But this code should also work as it works if I run it with node ./playground/ssr/src/circular-import/index.js (the .js extension needs to be appended to imports).

I guess while this PR fixes in some cases, it breaks in some cases.

I don't know a way to fix this completely but I feel I definitely need to read https://262.ecma-international.org/14.0/#sec-example-cyclic-module-record-graphs

related: #14048 (comment)

[Dunqing]

Good catch. Yes, It's a little tricky to fix completely. I'm guessing it would need to be in two phases as you said in #14048 (comment)

For this PR, I think the current import/export order is more intuitive.

[bluwy]

I've discussed this with Sapphi. While we'd like this fix too, the current PR does improves some areas with circular imports and doesn't regress this case (getB is not a function would also happen in main). So we can defer this to later and get this PR merged for now.

[bluwy]

Putting this out of 5.0 milestone for now as this fix could be done in a patch in the future. We're trying to scope down the milestone to release on time.

[Dunqing]

Putting this out of 5.0 milestone for now as this fix could be done in a patch in the future. We're trying to scope down the milestone to release on time.

What time for release 5.0?

[bluwy]

The schedule now is by the end of the month, so trying to finish up all of those in the milestone while having enough time for user testing.

[Dunqing]

The schedule now is by the end of the month, so trying to finish up all of those in the milestone while having enough time for user testing.

Thanks to the vite team for all their hard work and I can't wait for the release of vite 5!

[lysekjan]

Hello, any info about this issue?
I trying it on [email protected] and [email protected] but it seems the the bug is still there. Or am I doing something wrong?

https://github.com/lysekjan/vitest-imports

Version

vitest 1.0.4
vite 5.0.7