fix(ssr): stacktrace uses abs path with or without sourcemap #12902

fi3ework · 2023-04-18T19:22:13Z

Description

fix #12885.

add sourceRoot for better resolving the absolute source path which could helping resolve absolute stack trace
make all stack trace using an absolute path.

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 · 2023-04-18T19:22:22Z

Run & review this pull request in StackBlitz Codeflow.

fi3ework · 2023-04-18T19:23:47Z

playground/ssr-html/__tests__/ssr-html.spec.ts

+        const reg = new RegExp(
+          // TODO: ts without sourcemaps will resolve column to 8 which should be 9
+          path.resolve(__dirname, '../src') + '/error\\.' + ext + ':2:[89]',
+        )


not sure why this happened, we could fix this in another PR. I think it's not introduced here. 😅

sheremet-va · 2023-04-27T04:34:03Z

playground/css-sourcemap/__tests__/css-sourcemap.spec.ts

@@ -69,6 +69,7 @@ describe.runIf(isServe)('serve', () => {
    expect(formatSourcemapForSnapshot(map)).toMatchInlineSnapshot(`
      {
        "mappings": "AAAA;EACE,UAAU;AACZ;;ACAA;EACE,UAAU;AACZ",
+        "sourceRoot": "/root",


I don't think this is correct? The file is absolute, so it might work for the browser (as a pathname), but this will be incorrect in Node.js (should be a filepath).

Why do we need to specify sourceRoot anyway?

When the sources are relative paths, provide a sourceRoot to assemble the absolute paths. I think this is useful for user to locate the error source.

The throw error before PR, I think /src/error.js:2:9 is not easy to find the source file especially when using relative path.

# sourcemaps enabled: true Error: e at Module.error (/src/error.js:2:9) at file:///Users/fi3ework/Downloads/vite-main/playground/ssr-html/test-stacktrace.js:36:7

After PR:
All types of imports will using an absolute path in error stack.

# sourcemaps enabled: true # source file extension: js Error: e at Module.error (/Users/fi3ework/OSS/vite/playground/ssr-html/src/error.js:2:9) at file:///Users/fi3ework/OSS/vite/playground/ssr-html/test-stacktrace.js:44:9 Error: e at Module.error (/Users/fi3ework/OSS/vite/playground/ssr-html/src/error.js:2:9) at file:///Users/fi3ework/OSS/vite/playground/ssr-html/test-stacktrace.js:44:9 Error: e at Module.error (/Users/fi3ework/OSS/vite/playground/ssr-html/src/error.js:2:9) at file:///Users/fi3ework/OSS/vite/playground/ssr-html/test-stacktrace.js:44:9

Source route doesn't have to be absolute as far as I know.

In your example code will not work correctly in SSR because it's not absolute to the file system.

How does it know that /root is actually relative to the root of the project and not an absolute path on the file system?

Source route doesn't have to be absolute as far as I know.

Yeah, it doesn't have to be. But it will be easy to locate the source file IMO. Does this behavior break something I don't know? 🤔

In your example code will not work correctly in SSR because it's not absolute to the file system.

The example generated by running pnpm test-serve -t stacktrace in playground/ssr-html. Could you explain in more detail, Thanks!

How does it know that /root is actually relative to the root of the project and not an absolute path on the file system?

/root is replaced by an absolute path in E2E test to normalize the actual paths in different runners. If not matched, it should keep as-is.

@fi3ework Would changing sourceURL=${mod.url} to sourceURL=${mod.file} work?
I guess Node.js is treating the original file path as /src/error.js. We want Node.js to treat the original file path as /Users/fi3ework/OSS/vite/playground/ssr-html/src/error.js (or file:///Users/fi3ework/OSS/vite/playground/ssr-html/src/error.js).

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

Line 225 in a8c7eb2

`\n//# sourceURL=${mod.url}${sourceMapSuffix}`,

/root is replaced by an absolute path in E2E test to normalize the actual paths in different runners. If not matched, it should keep as-is.

I see then. I assumed it replaces /src/file.ts with <root>/file.ts, and not <fsRootPath>/src/file.ts. It should be fine from my side then.

@sapphi-red Basically could work except for Windows 😅. Check out https://github.com/vitejs/vite/actions/runs/4939126748/jobs/8829577947?pr=12902#step:14:50. The error location was incorrectly resolved to 4 instead of 2 for two cases (others are good). I don't think I can fix it quickly without a Windows device. 😅 (had reverted the tweak)

Thanks for testing 💚

According to the sourcemap spec, when the sources are relative paths, it will be resolved from the place of the file. So it should work without specifing sourceRoot. Also setting sourceRoot will cause a problem that #12079 solved.

Vite was lacking support for resolving relative sources and that was causing the stacktrace to be like that. I pushed some commits implementing that. Coincidentally I found the reason of #12902 (comment) and fixed that too. It would be nice if you could review my change.

Using id is a much more clever solution than url. Nice work! LGTM.

This reverts commit 8d2075b.

This reverts commit fccb8f9.

sapphi-red · 2023-05-13T16:57:42Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-05-13T16:58:03Z

📝 Ran ecosystem CI: Open

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

patak-dev · 2023-05-15T10:20:17Z

The Nuxt issue is the same one we have currently in main, let's merge this one for the next patch

fi3ework commented Apr 18, 2023

View reviewed changes

fi3ework force-pushed the fix-stacktrace branch 2 times, most recently from 093de39 to 90ca10f Compare April 18, 2023 19:37

This comment was marked as resolved.

Sign in to view

fi3ework force-pushed the fix-stacktrace branch 7 times, most recently from 2e69cb8 to 6772daf Compare April 19, 2023 18:38

fi3ework requested a review from sapphi-red April 19, 2023 18:50

sheremet-va reviewed Apr 27, 2023

View reviewed changes

fi3ework added 3 commits May 10, 2023 21:00

fix(ssr): stacktrace uses abs path with or without sourcemap

5beaf20

fix: Windows path

6521e96

prefer mod.file

8d2075b

fi3ework force-pushed the fix-stacktrace branch from 6772daf to 8d2075b Compare May 10, 2023 15:56

fi3ework and others added 6 commits May 11, 2023 00:12

test windows path

fccb8f9

Revert "prefer mod.file"

9a3c3c4

This reverts commit 8d2075b.

Revert "test windows path"

06301b7

This reverts commit fccb8f9.

chore: revert changes

a1bebbc

fix: handle relative path sourcemap correctly

51dae22

fix: sourcemap stacktrace difference

89b865e

sapphi-red approved these changes May 13, 2023

View reviewed changes

sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr labels May 13, 2023

patak-dev merged commit 88c855e into vitejs:main May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ssr): stacktrace uses abs path with or without sourcemap #12902

fix(ssr): stacktrace uses abs path with or without sourcemap #12902

fi3ework commented Apr 18, 2023

stackblitz bot commented Apr 18, 2023

fi3ework Apr 18, 2023

This comment was marked as resolved.

sheremet-va Apr 27, 2023

fi3ework May 1, 2023

sheremet-va May 1, 2023

fi3ework May 1, 2023 •

edited

Loading

sapphi-red May 10, 2023

sheremet-va May 10, 2023

fi3ework May 10, 2023

sapphi-red May 13, 2023 •

edited

Loading

fi3ework May 13, 2023

sapphi-red commented May 13, 2023

vite-ecosystem-ci bot commented May 13, 2023 •

edited

Loading

patak-dev commented May 15, 2023

fix(ssr): stacktrace uses abs path with or without sourcemap #12902

fix(ssr): stacktrace uses abs path with or without sourcemap #12902

Conversation

fi3ework commented Apr 18, 2023

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

stackblitz bot commented Apr 18, 2023

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fi3ework May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapphi-red May 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapphi-red commented May 13, 2023

vite-ecosystem-ci bot commented May 13, 2023 • edited Loading

patak-dev commented May 15, 2023

fi3ework May 1, 2023 •

edited

Loading

sapphi-red May 13, 2023 •

edited

Loading

vite-ecosystem-ci bot commented May 13, 2023 •

edited

Loading