fix(ssrTransform): preserve line offset when transforming imports #19004

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)

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
16 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

Open

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

Draft

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ssrTransform): preserve line offset when transforming imports #19004

fix(ssrTransform): preserve line offset when transforming imports #19004

aleclarson commented Dec 18, 2024

sapphi-red left a comment

This comment was marked as outdated.

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

edited

Loading

This comment was marked as outdated.

sapphi-red commented Dec 19, 2024

aleclarson commented Dec 19, 2024

aleclarson Dec 19, 2024

aleclarson Dec 19, 2024

aleclarson Dec 19, 2024

sapphi-red left a comment

sapphi-red commented Dec 20, 2024

vite-ecosystem-ci bot commented Dec 20, 2024

sapphi-red commented Dec 23, 2024

hi-ogawa left a comment

fix(ssrTransform): preserve line offset when transforming imports #19004

fix(ssrTransform): preserve line offset when transforming imports #19004

Conversation

aleclarson commented Dec 18, 2024

Description

Broken import hoisting

Preserving line breaks

Why should line offset be preserved?

sapphi-red left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

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

This comment was marked as outdated.

sapphi-red commented Dec 19, 2024

aleclarson commented Dec 19, 2024

aleclarson Dec 19, 2024

Choose a reason for hiding this comment

aleclarson Dec 19, 2024

Choose a reason for hiding this comment

aleclarson Dec 19, 2024

Choose a reason for hiding this comment

sapphi-red left a comment

Choose a reason for hiding this comment

sapphi-red commented Dec 20, 2024

vite-ecosystem-ci bot commented Dec 20, 2024

sapphi-red commented Dec 23, 2024

hi-ogawa left a comment

Choose a reason for hiding this comment

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

edited

Loading