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

chore: revert "fix(ssr): hoist import statements to the top (#12274)" #12527

Closed

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Mar 22, 2023

Description

revert #12274

#12274 seems to break nuxt, plugin-react ecosystem-ci.
This PR fixes the other half part of the fails.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Mar 22, 2023

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

@sapphi-red
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 22, 2023

📝 Ran ecosystem CI: Open

suite result
astro ❌ failure
histoire ❌ failure
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ✅ 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
windicss ✅ success

@bluwy
Copy link
Member

bluwy commented Mar 22, 2023

I wonder if we should try to fix this first since the bug has not been released yet. Using the test from #12528,

export * from './module-a.js'
export { getValueAB } from './module-b.js'

Reverting this fixes it since we provide an export value from './module-a.js' before './module-b.js' access it. It also runs well in Node. However this seems to be unique to export * only. If I change this to:

export const valueA = 'circ-dep-init-a'
export { getValueAB } from './module-b.js'

It errors in Node. Testing things around, e.g. with:

 const foo = 'bar'
 export {foo}

 export * from './module-a.js'

console.log(getValueAB())
import { getValueAB } from './module-b.js' // <-- also tries to read `foo`, but fails

I think the rule is that:

  1. re-exports gets hoisted to the highest level
  2. then hoist import statements

Right now this order is not guaranteed, I'm testing a fix locally for now.

@patak-dev
Copy link
Member

Closing as #12530 has been merged

@patak-dev patak-dev closed this Mar 22, 2023
@sapphi-red sapphi-red deleted the chore/revert-hoist-import-ssr branch March 22, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants