fix(ssrTransform): preserve line offset when transforming imports by aleclarson · Pull Request #19004 · vitejs/vite

aleclarson · 2024-12-18T17:35:56Z

Description

The ssrTransformScript function doesn‘t preserve line numbers, and there are two reasons for this:

Broken import hoisting
Not preserving the number of line breaks in the import statement being transformed

Broken import hoisting

Imports were being hoisted when it wasn't necessary. This was due to a \n character between imports and the fact that importNode.end does not include the trailing line break.

I've changed this logic to only hoist imports when a non-whitespace character is found between the current import and the previous one. Note that this won't account for JavaScript comments between import statements, but that was already the case. I've added a TODO comment about this edge case.

Preserving line breaks

Often times, an import statement will span multiple lines, particularly when using named exports.

import {
    x,
    y,
    z
} from 'xyz'

The SSR transform was not respecting this. I've changed the logic to include extra \n characters at the end of a transformed import to ensure the line offset is preserved.

Why should line offset be preserved?

The main reason to preserve line offset is it avoids the need for sourcemaps. Vitest prefers not to add sourceMappingURL comments to transformed modules from a node_modules directory, resulting in a poor debugging experience (see vitest-dev/vitest#5605). But it wouldn't be necessary to apply a sourcemap if the line offset was just preserved.

While one could argue that you shouldn't be stepping into an installed package, people do it nonetheless, and it's not a big ask for the experience to not be completely broken by Vite's SSR transform.

sapphi-red

I think this makes sense.
Would you add a test case for "Broken import hoisting"? It'd be also nice if there's a skipped test case for the edge case related to the comment.

pkg-pr-new · 2024-12-19T02:11:08Z

Open in Stackblitz

npm i https://pkg.pr.new/vite@19004

commit: 684893d

sapphi-red · 2024-12-19T02:55:41Z

The failure of laravel and marko is a known issue on ecosystem-ci side.
The failure of Vitest is caused by the increase of coverage in this snapshot: https://github.com/vitest-dev/vitest/blob/5ba49475dc1efe10a4d43443f70c683b9fde8720/test/coverage-test/test/vue.test.ts#L30-L45

aleclarson · 2024-12-19T15:30:52Z

@sapphi-red Good call to write that test, as there were bugs 🙈

aleclarson · 2024-12-19T15:31:52Z

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

+test('preserve line offset when rewriting imports', async () => {
+  // The line number of each non-import statement must not change.
+  const inputLines = [
+    `debugger;`,
+    ``,
+    `import {`,
+    `  dirname,`,
+    `  join,`,
+    `} from 'node:path';`,
+    ``,
+    `debugger;`,
+    ``,
+    `import fs from 'node:fs';`,
+    ``,
+    `debugger;`,
+    ``,
+    `import {`,
+    `  red,`,
+    `  green,`,
+    `} from 'kleur/colors';`,
+    ``,
+    `debugger;`,
+  ]
+
+  const output = await ssrTransformSimpleCode(inputLines.join('\n'))
+  expect(output).toBeDefined()
+
+  const outputLines = output!.split('\n')
+  expect(
+    outputLines
+      .map((line, i) => `${String(i + 1).padStart(2)} | ${line}`.trimEnd())
+      .join('\n'),
+  ).toMatchInlineSnapshot(`
+    " 1 | const __vite_ssr_import_0__ = await __vite_ssr_import__("node:path", {"importedNames":["dirname","join"]});const __vite_ssr_import_1__ = await __vite_ssr_import__("node:fs", {"importedNames":["default"]});const __vite_ssr_import_2__ = await __vite_ssr_import__("kleur/colors", {"importedNames":["red","green"]});debugger;
+     2 |
+     3 |
+     4 |
+     5 |
+     6 |
+     7 |
+     8 | debugger;
+     9 |
+    10 |
+    11 |
+    12 | debugger;
+    13 |
+    14 |
+    15 |
+    16 |
+    17 |
+    18 |
+    19 | debugger;"
+  `)
+
+  // Ensure the debugger statements are still on the same lines.
+  expect(outputLines[0].endsWith(inputLines[0])).toBe(true)
+  expect(outputLines[7]).toBe(inputLines[7])
+  expect(outputLines[11]).toBe(inputLines[11])
+  expect(outputLines[18]).toBe(inputLines[18])
+})


New test is here!

aleclarson · 2024-12-19T15:34:29Z

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

+test.fails('comments between imports do not trigger hoisting', async () => {
+  expect(
+    await ssrTransformSimpleCode(
+      `import { dirname } from 'node:path';// comment\nimport fs from 'node:fs';`,
+    ),
  ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = await __vite_ssr_import__("node:path", {"importedNames":["default"]});
-    __vite_ssr_import_0__.default.resolve('server.js');"
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__("node:path", {"importedNames":["dirname"]});// comment
+    const __vite_ssr_import_1__ = await __vite_ssr_import__("node:fs", {"importedNames":["default"]});"
  `)
 })


The failing “comment between imports” hoisting test.

aleclarson · 2024-12-19T15:35:08Z

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

+test('do not hoist import if only whitespace is between them', async () => {
+  expect(
+    await ssrTransformSimpleCode(
+      `import { dirname } from 'node:path';\n\n\nimport fs from 'node:fs';`,
+    ),
+  ).toMatchInlineSnapshot(`
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__("node:path", {"importedNames":["dirname"]});
+
+
+    const __vite_ssr_import_1__ = await __vite_ssr_import__("node:fs", {"importedNames":["default"]});"
+  `)
+})


Finally, here's a basic “preserve whitespace between imports” hoisting test.

sapphi-red

Thanks for the tests!

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

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

sapphi-red · 2024-12-20T04:16:28Z

/ecosystem-ci run

vite-ecosystem-ci · 2024-12-20T04:49:07Z

📝 Ran ecosystem CI on 0317abd: Open

suite	result	latest scheduled
histoire	❌ failure	❌ failure
laravel	❌ failure	✅ success
marko	❌ failure	✅ success
nuxt	❌ failure	✅ success
redwoodjs	❌ failure	❌ failure
remix	❌ failure	❌ failure
vike	✅ success	❌ failure
vite-plugin-svelte	❌ failure	❌ failure
vitest	❌ failure	✅ success
waku	❌ failure	❌ failure

✅ analogjs, astro, ladle, previewjs, quasar, qwik, rakkas, storybook, sveltekit, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

Co-authored-by: 翠 / green <[email protected]>

sapphi-red · 2024-12-23T05:48:33Z

@hi-ogawa Would you take a look if the coverage change in the Vitest suite in the ecosystem-ci makes sense?

hi-ogawa

Looks good to me!
I checked Vitest failures in vitest-dev/vitest#7124 and changes are mostly irrelevant for users and actually some bugs are on Vitest side, so it should be fine to proceed with this PR.
I'll follow up with Vitest side fix later to pass ecosystem CI.

| datasource | package | from | to | | ---------- | ------- | ----- | ----- | | npm | vite | 6.0.5 | 6.0.6 | ## [v6.0.6](https://github.com/vitejs/vite/blob/HEAD/packages/vite/CHANGELOG.md#small606-2024-12-26-small) - fix: replace runner-side path normalization with `fetchModule`-side resolve ([#18361](vitejs/vite#18361)) ([9f10261](vitejs/vite@9f10261)), closes [#18361](vitejs/vite#18361) - fix(css): resolve style tags in HTML files correctly for lightningcss ([#19001](vitejs/vite#19001)) ([afff05c](vitejs/vite@afff05c)), closes [#19001](vitejs/vite#19001) - fix(css): show correct error when unknown placeholder is used for CSS modules pattern in lightningcs ([9290d85](vitejs/vite@9290d85)), closes [#19070](vitejs/vite#19070) - fix(resolve): handle package.json with UTF-8 BOM ([#19000](vitejs/vite#19000)) ([902567a](vitejs/vite@902567a)), closes [#19000](vitejs/vite#19000) - fix(ssrTransform): preserve line offset when transforming imports ([#19004](vitejs/vite#19004)) ([1aa434e](vitejs/vite@1aa434e)), closes [#19004](vitejs/vite#19004) - chore: fix typo in comment ([#19067](vitejs/vite#19067)) ([eb06ec3](vitejs/vite@eb06ec3)), closes [#19067](vitejs/vite#19067) - chore: update comment about `build.target` ([#19047](vitejs/vite#19047)) ([0e9e81f](vitejs/vite@0e9e81f)), closes [#19047](vitejs/vite#19047) - revert: unpin esbuild version ([#19043](vitejs/vite#19043)) ([8bfe247](vitejs/vite@8bfe247)), closes [#19043](vitejs/vite#19043) - test(ssr): test virtual module with query ([#19044](vitejs/vite#19044)) ([a1f4b46](vitejs/vite@a1f4b46)), closes [#19044](vitejs/vite#19044)

…tejs#19004) Co-authored-by: 翠 / green <[email protected]>

fix(ssrTransform): preserve line offset when transforming imports

5f10b65

aleclarson mentioned this pull request Dec 18, 2024

feat: Add deps.includeSourcemap vitest-dev/vitest#7095

Closed

6 tasks

aleclarson added 2 commits December 18, 2024 13:20

update test snapshots

b8c5ffe

chore: format

37586e1

sapphi-red added the feat: ssr label Dec 19, 2024

sapphi-red reviewed Dec 19, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

vite-ecosystem-ci bot added the trigger: preview label Dec 19, 2024

This comment was marked as outdated.

Sign in to view

aleclarson added 2 commits December 19, 2024 10:27

test: import hoisting and line offset preservation

811683f

get the test to pass

2281311

aleclarson commented Dec 19, 2024

View reviewed changes

one more test

dbd8e54

aleclarson commented Dec 19, 2024

View reviewed changes

tweak test name

0317abd

sapphi-red reviewed Dec 20, 2024

View reviewed changes

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

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

aleclarson and others added 2 commits December 20, 2024 09:52

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

defd7f4

Co-authored-by: 翠 / green <[email protected]>

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

684893d

Co-authored-by: 翠 / green <[email protected]>

aleclarson requested a review from sapphi-red December 20, 2024 15:15

sapphi-red approved these changes Dec 23, 2024

View reviewed changes

hi-ogawa mentioned this pull request Dec 24, 2024

fix(vite-node): remove fake first line mapping on Vite 6 vitest-dev/vitest#7124

Merged

6 tasks

hi-ogawa approved these changes Dec 24, 2024

View reviewed changes

sapphi-red merged commit 1aa434e into vitejs:main Dec 24, 2024
15 checks passed

sapphi-red mentioned this pull request Dec 25, 2024

perf(css): only run postcss when needed #19061

Merged

This was referenced Dec 25, 2024

Missing function coverage when transform prepends code right before the function vitest-dev/vitest#7130

Closed

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

Merged

bgoscinski mentioned this pull request Jan 2, 2025

Source maps in inlined deps are off by a few lines in VS Code vitest-dev/vitest#5605

Closed

6 tasks

Dunqing mentioned this pull request Feb 18, 2025

transformer: vite ssrTransform oxc-project/oxc#9169

Closed

hi-ogawa mentioned this pull request Feb 25, 2025

fix(vite-node): fix source map of inlined node_modules vitest-dev/vitest#7557

Merged

6 tasks

hi-ogawa mentioned this pull request Apr 10, 2025

refactor(ssr): remove ssrTransform line offset preservation #19829

Merged

moonlitusun pushed a commit to moonlitusun/vite that referenced this pull request May 25, 2025

fix(ssrTransform): preserve line offset when transforming imports (vi…

0f143a1

…tejs#19004) Co-authored-by: 翠 / green <[email protected]>

Uh oh!

Comments

Conversation

aleclarson commented Dec 18, 2024

Description

Broken import hoisting

Preserving line breaks

Why should line offset be preserved?

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

pkg-pr-new bot commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

sapphi-red commented Dec 19, 2024

Uh oh!

aleclarson commented Dec 19, 2024

Uh oh!

aleclarson Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

aleclarson Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

aleclarson Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sapphi-red commented Dec 20, 2024

Uh oh!

vite-ecosystem-ci bot commented Dec 20, 2024

Uh oh!

sapphi-red commented Dec 23, 2024

Uh oh!

hi-ogawa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pkg-pr-new bot commented Dec 19, 2024 •

edited

Loading