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): crash on circular import #14441

Merged
merged 9 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 7 additions & 6 deletions packages/vite/src/node/ssr/__tests__/ssrTransform.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,9 @@ test('export * from', async () => {
).toMatchInlineSnapshot(`
"const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"vue\\");
__vite_ssr_exportAll__(__vite_ssr_import_0__);

const __vite_ssr_import_1__ = await __vite_ssr_import__(\\"react\\");
__vite_ssr_exportAll__(__vite_ssr_import_1__);

"
`)
})
Expand Down Expand Up @@ -940,9 +940,10 @@ test('import assertion attribute', async () => {
import('./bar.json', { assert: { type: 'json' } });
`),
).toMatchInlineSnapshot(`
"const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"./foo.json\\");


__vite_ssr_dynamic_import__('./bar.json', { assert: { type: 'json' } });
"`)
"const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"./foo.json\\");


__vite_ssr_dynamic_import__('./bar.json', { assert: { type: 'json' } });
"
`)
})
20 changes: 8 additions & 12 deletions packages/vite/src/node/ssr/ssrTransform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,11 @@ async function ssrTransformScript(
// hoist at the start of the file, after the hashbang
const hoistIndex = code.match(hashbangRE)?.[0].length ?? 0

function defineImport(source: string) {
function defineImport(index = 0, source: string) {
Dunqing marked this conversation as resolved.
Show resolved Hide resolved
deps.add(source)
const importId = `__vite_ssr_import_${uid++}__`
// There will be an error if the module is called before it is imported,
// so the module import statement is hoisted to the top
s.appendLeft(
hoistIndex,
index,
`const ${importId} = await ${ssrImportKey}(${JSON.stringify(source)});\n`,
)
return importId
Expand All @@ -124,7 +122,7 @@ async function ssrTransformScript(
// import { baz } from 'foo' --> baz -> __import_foo__.baz
// import * as ok from 'foo' --> ok -> __import_foo__
if (node.type === 'ImportDeclaration') {
const importId = defineImport(node.source.value as string)
const importId = defineImport(hoistIndex, node.source.value as string)
s.remove(node.start, node.end)
for (const spec of node.specifiers) {
if (spec.type === 'ImportSpecifier') {
Expand Down Expand Up @@ -167,11 +165,10 @@ async function ssrTransformScript(
s.remove(node.start, node.end)
if (node.source) {
// export { foo, bar } from './foo'
const importId = defineImport(node.source.value as string)
// hoist re-exports near the defined import so they are immediately exported
const importId = defineImport(node.start, node.source.value as string)
for (const spec of node.specifiers) {
defineExport(
hoistIndex,
node.start,
spec.exported.name,
`${importId}.${spec.local.name}`,
)
Expand Down Expand Up @@ -217,12 +214,11 @@ async function ssrTransformScript(
// export * from './foo'
if (node.type === 'ExportAllDeclaration') {
s.remove(node.start, node.end)
const importId = defineImport(node.source.value as string)
// hoist re-exports near the defined import so they are immediately exported
const importId = defineImport(node.start, node.source.value as string)
if (node.exported) {
defineExport(hoistIndex, node.exported.name, `${importId}`)
defineExport(node.start, node.exported.name, `${importId}`)
} else {
s.appendLeft(hoistIndex, `${ssrExportAllKey}(${importId});\n`)
s.appendLeft(node.start, `${ssrExportAllKey}(${importId});\n`)
}
}
}
Expand Down
8 changes: 8 additions & 0 deletions playground/ssr/__tests__/ssr.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ test(`circular dependencies modules doesn't throw`, async () => {
)
})

test(`circular import doesn't throw`, async () => {
await page.goto(`${url}/circular-import`)

expect(await page.textContent('.circ-import')).toMatchInlineSnapshot(
'"A is: __A__"',
)
})

test(`deadlock doesn't happen`, async () => {
await page.goto(`${url}/forked-deadlock`)

Expand Down
6 changes: 6 additions & 0 deletions playground/ssr/src/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { escapeHtml } from './utils'
const pathRenderers = {
'/': renderRoot,
'/circular-dep': renderCircularDep,
'/circular-import': renderCircularImport,
'/forked-deadlock': renderForkedDeadlock,
}

Expand Down Expand Up @@ -34,6 +35,11 @@ async function renderCircularDep(rootDir) {
return `<div class="circ-dep-init">${escapeHtml(getValueAB())}</div>`
}

async function renderCircularImport(rootDir) {
const { logA } = await import('./circular-import/index.js')
return `<div class="circ-import">${escapeHtml(logA())}</div>`
}

async function renderForkedDeadlock(rootDir) {
const { commonModuleExport } = await import('./forked-deadlock/common-module')
commonModuleExport()
Expand Down
5 changes: 5 additions & 0 deletions playground/ssr/src/circular-import/a.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { getB } from './b'

export const A = '__A__'

export const B = getB()
5 changes: 5 additions & 0 deletions playground/ssr/src/circular-import/b.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export function getB() {
return '__B__'
}

export { A } from './a'
Comment on lines +1 to +5
Copy link
Member

@sapphi-red sapphi-red Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

5 changes: 5 additions & 0 deletions playground/ssr/src/circular-import/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { A } from './b'

export function logA() {
return `A is: ${A}`
}