Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ssr): hoist import statements to the top #12274

Merged
merged 3 commits into from Mar 21, 2023

Conversation

btea
Copy link
Collaborator

@btea btea commented Mar 3, 2023

Description

fix #10444

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
Copy link

stackblitz bot commented Mar 3, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

packages/vite/src/node/ssr/ssrTransform.ts Show resolved Hide resolved
packages/vite/src/node/ssr/ssrTransform.ts Outdated Show resolved Hide resolved
packages/vite/src/node/ssr/ssrTransform.ts Outdated Show resolved Hide resolved
@bluwy bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr labels Mar 7, 2023
@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 8, 2023

📝 Ran ecosystem CI: Open

suite result
astro ❌ failure
histoire ❌ failure
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ❌ failure
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ❌ failure
vite-plugin-react ❌ failure
vite-plugin-react-pages ❌ failure
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ❌ failure
windicss ✅ success

@patak-dev
Copy link
Member

cc @sheremet-va, I don't think this would affect vite-node but good to be aware of this change.

@sheremet-va
Copy link
Member

cc @sheremet-va, I don't think this would affect vite-node but good to be aware of this change.

Will not affect vite-node, but will break Vitest module mocking (vi.mock) 😄

@patak-dev
Copy link
Member

I need the 😬 emoji reaction to add to your response. How do we handle this merge to avoid issues in Vitest? Could vi.mock be updated to cope with this change and we delay merging until 4.3?

@sheremet-va
Copy link
Member

Could vi.mock be updated to cope with this change and we delay merging until 4.3?

There is a way to make it work with both versions, yes. Previously it relied on a behavior that SSR transform will not affect imports position, so "vi.mock" was hoisted before any "import" in the source code, and not in the transformed code and it was done as a Vite plugin.

Now we will need to move it after the code is transformed and also somehow merge source maps, which was handled by Vite previously.

@patak-dev patak-dev added this to the 4.3 milestone Mar 8, 2023
@patak-dev
Copy link
Member

@sheremet-va do you think we can move forward with this PR in 4.3? The stable cut for it may be released in 2-3 weeks.

@sheremet-va
Copy link
Member

@sheremet-va do you think we can move forward with this PR in 4.3? The stable cut for it may be released in 2-3 weeks.

Should be fine if CI is passing

@patak-dev
Copy link
Member

/ecosystem-ci run vitest

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 21, 2023

📝 Ran ecosystem CI: Open

suite result
vitest ✅ success

@patak-dev patak-dev enabled auto-merge (squash) March 21, 2023 09:12
@patak-dev patak-dev merged commit 33baff5 into vitejs:main Mar 21, 2023
11 checks passed
@btea btea deleted the fix/ssr-transform branch March 21, 2023 09:20
sapphi-red added a commit to sapphi-red/vite that referenced this pull request Mar 22, 2023
@bluwy bluwy mentioned this pull request Sep 25, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSR transform doesn't respect hoisting
5 participants