fix(ssr): crash on circular import by Dunqing · Pull Request #14441 · vitejs/vite

Dunqing · 2023-09-22T09:41:23Z

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.

bolt-new-by-stackblitz · 2023-09-22T09:41:25Z

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.

packages/vite/src/node/ssr/ssrTransform.ts

rtsao

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

Co-authored-by: Bjorn Lu <[email protected]>

Dunqing · 2023-10-07T00:58:57Z

I think this PR can be merged first

bluwy · 2023-10-09T06:36:57Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-10-09T06:37:26Z

📝 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 · 2023-10-09T07:52:44Z

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 · 2023-10-09T07:57:27Z

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 · 2023-10-09T08:07:21Z

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 · 2023-10-09T09:25:46Z

playground/ssr/src/circular-import/b.js

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


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)

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.

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 · 2023-10-13T15:42:28Z

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 · 2023-10-13T16:27:02Z

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 · 2023-10-13T16:36:51Z

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 · 2023-10-13T16:57:16Z

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 · 2023-12-12T08:05:48Z

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

test: test case ready

40b670f

fix(ssr): crash on circular import

a891348

Dunqing changed the title ~~fix(ssr): crash for circular import~~ fix(ssr): crash on circular import Sep 22, 2023

fix: remove dead code

6ba5930

Dunqing marked this pull request as ready for review September 22, 2023 12:26

rtsao mentioned this pull request Sep 22, 2023

test(ssr): proper test coverage of SSR shebang import hoisting #14448

Merged

9 tasks

Dunqing added 2 commits September 23, 2023 09:09

refactor: hoist imports

241ed9f

test: update

074e608

Dunqing mentioned this pull request Sep 25, 2023

feat(ssr): reusing imported imports #14456

Closed

9 tasks

bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr labels Sep 25, 2023

bluwy reviewed Sep 25, 2023

View reviewed changes

packages/vite/src/node/ssr/ssrTransform.ts Outdated Show resolved Hide resolved

bluwy mentioned this pull request Sep 25, 2023

test(ssr): add import and export ordering snapshot #14468

Merged

4 tasks

Dunqing added 2 commits September 26, 2023 20:43

Merge branch 'main' into fix/issue/14010

4de2821

test: update snap

c3bb8a7

rtsao approved these changes Oct 6, 2023

View reviewed changes

Update packages/vite/src/node/ssr/ssrTransform.ts

df08ae8

Co-authored-by: Bjorn Lu <[email protected]>

bluwy previously approved these changes Oct 9, 2023

View reviewed changes

bluwy added this to the 5.0 milestone Oct 9, 2023

sapphi-red reviewed Oct 9, 2023

View reviewed changes

bluwy removed this from the 5.0 milestone Oct 13, 2023

patak-dev added this to the 5.2 milestone Feb 12, 2024

Merge branch 'main' into pr/Dunqing/14441

6163a85

bluwy dismissed their stale review via 6163a85 March 12, 2024 14:41

bluwy approved these changes Mar 13, 2024

View reviewed changes

bluwy merged commit 8cd846c into vitejs:main Mar 13, 2024

Dunqing mentioned this pull request Mar 23, 2024

chore: update vite v5.2 vitest-dev/vitest#5421

Closed

6 tasks

hi-ogawa mentioned this pull request Oct 18, 2024

fix(ssr): preserve source maps for hoisted imports (fix #16355) #16356

Merged

sapphi-red mentioned this pull request Jan 7, 2025

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

Merged

3 tasks

This was referenced Apr 11, 2025

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

Closed

fix(ssr): fix execution order of re-export #19841

Merged

Uh oh!

Comments

Conversation

Dunqing commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

What is the purpose of this pull request?

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

Uh oh!

bolt-new-by-stackblitz bot commented Sep 22, 2023

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rtsao left a comment

Choose a reason for hiding this comment

Uh oh!

Dunqing commented Oct 7, 2023

Uh oh!

bluwy commented Oct 9, 2023

Uh oh!

vite-ecosystem-ci bot commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluwy commented Oct 9, 2023

Uh oh!

Dunqing commented Oct 9, 2023

Uh oh!

bluwy commented Oct 9, 2023

Uh oh!

sapphi-red Oct 9, 2023 • 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!

Dunqing Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

bluwy Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

bluwy commented Oct 13, 2023

Uh oh!

Dunqing commented Oct 13, 2023

Uh oh!

bluwy commented Oct 13, 2023

Uh oh!

Dunqing commented Oct 13, 2023

Uh oh!

lysekjan commented Dec 12, 2023

Version

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Dunqing commented Sep 22, 2023 •

edited

Loading

vite-ecosystem-ci bot commented Oct 9, 2023 •

edited

Loading

sapphi-red Oct 9, 2023 •

edited

Loading