Skip to content

Comments

fix(ssr): load sourcemaps alongside modules#11780

Merged
patak-dev merged 8 commits intovitejs:mainfrom
sapphi-red:fix/ssr-load-sourcemaps
Apr 4, 2023
Merged

fix(ssr): load sourcemaps alongside modules#11780
patak-dev merged 8 commits intovitejs:mainfrom
sapphi-red:fix/ssr-load-sourcemaps

Conversation

@sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Jan 22, 2023 •
edited
Loading

Description

This PR reapplies #11576 with a fix and tests.
Also this PR adds some comments why this works.

fixes #3288

cc @patak-dev @Vap0r1ze @benmccann

Additional context

#11576 requires source map support for new Function which landed in Node.js "^16.17.0 || >=18.6.0".
Adding the offset seems to be required due to V8's bug (nodejs/node#43047 (comment)). When the bug is fixed, that part of the code needs to be removed.

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.

@sapphi-red sapphi-red changed the title Fix/ssr load sourcemaps fix(ssr): load sourcemaps alongside modules Jan 22, 2023
@sapphi-red
Copy link
Member Author

sapphi-red commented Jan 22, 2023

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Jan 22, 2023 •
edited
Loading

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ❌ failure
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ✅ success
previewjs ❌ failure
qwik ❌ failure
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ❌ failure
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

@Vap0r1ze
Copy link
Contributor

Vap0r1ze commented Jan 22, 2023

Will this work if I set a breakpoint on the first line of a file?

@sapphi-red
Copy link
Member Author

sapphi-red commented Jan 22, 2023

I think it doesn't. I guess we can append 'use strict' in ssrTransformScript. But I'm not sure if we can introduce that change in a minor version bump.
related: #9199 (comment)

sapphi-red
sapphi-red commented Jan 22, 2023
View reviewed changes
playground/ssr-html/test-stacktrace.js

const isSourceMapEnabled = process.argv[2] === 'true'
process.setSourceMapsEnabled(isSourceMapEnabled)
console.log('# sourcemaps enabled:', isSourceMapEnabled)
Copy link
Member Author

@sapphi-red sapphi-red Jan 22, 2023

Choose a reason for hiding this comment

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

This won't work if something called process.setSourceMapsEnabled after this line. (I mean the isSourceMapEnabled will not be synced with the actual value.)

I've created a feature request to Node to fix this: nodejs/node#46304

@Vap0r1ze
Copy link
Contributor

Vap0r1ze commented Jan 22, 2023

Ah I see that makes sense, that's fine I think

@xania
Copy link

xania commented Feb 4, 2023

Hi, I've added a fix to the PR, you can check it out in
sapphi-red#5

@axe-me axe-me mentioned this pull request Feb 15, 2023
Open
patak-dev
patak-dev approved these changes Apr 4, 2023
View reviewed changes
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Let's merge this one so we can test it during the current beta.

@patak-dev patak-dev merged commit be95050 into vitejs:main Apr 4, 2023
@mengdu
Copy link

mengdu commented Apr 19, 2023

How to break points in vscode? Doesn't work on JavaScript Debug Terminal

@cyco130 cyco130 mentioned this pull request Apr 27, 2023
Merged
9 tasks
@cyco130 cyco130 mentioned this pull request May 29, 2023
Open
@aheissenberger aheissenberger mentioned this pull request Jun 3, 2023
Closed
@sapphi-red sapphi-red mentioned this pull request Jun 14, 2023
Closed
9 tasks
@sapphi-red sapphi-red deleted the fix/ssr-load-sourcemaps branch June 14, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patak-dev patak-dev patak-dev approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SSR SourceMap not loaded when debugging

5 participants

@sapphi-red @Vap0r1ze @xania @mengdu @patak-dev