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): load sourcemaps alongside modules #11780

Merged
merged 8 commits into from
Apr 4, 2023

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Jan 22, 2023

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

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Jan 22, 2023

📝 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

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

@sapphi-red
Copy link
Member Author

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)


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

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSR SourceMap not loaded when debugging
5 participants