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

feat(ssr): better DX with sourcemaps, breakpoints, error messages #3928

Closed
wants to merge 15 commits into from

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Jun 23, 2021

Description

This PR adds the following features to SSR only:

  • Breakpoints now open up the proper source file
  • Sourcemap chains are now used by stack traces
  • Use @babel/code-frame to show the snippet related to any error from instantiating a module.
  • In development only, modules are instantiated with vm.runInThisContext for breakpoint support.

Additional context

Breakpoints may still be buggy in some cases, due to an issue with ESBuild (see here).

Fixes #3288

cc @brillout

Todo

  • Update lockfile

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 Commit 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.

await injectSourcesContent(map, mod.file, moduleGraph)
}

ssrModuleImpl += `\n` + convertSourceMap.fromObject(map).toComment()
Copy link
Member Author

Choose a reason for hiding this comment

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

The inline sourcemap is appended in both production and development, as new Function still handles sourcemaps somewhat, but vm.runInThisContext actually respects breakpoints.

const { map } = result
if (map?.mappings) {
if (mod.file) {
await injectSourcesContent(map, mod.file, moduleGraph)
Copy link
Member Author

@aleclarson aleclarson Jun 24, 2021

Choose a reason for hiding this comment

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

Honestly not sure if this call is needed in production. Someone needs to test with and without to see the difference (as it may affect stack traces?). Otherwise, in development, this ensures that changes to local modules will not appear while debugging an earlier version of a module.

@aleclarson aleclarson added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jun 25, 2021
@brillout
Copy link
Contributor

Neat... I actually regularly run into problems with source maps.

It'd be lovely to have this merged although I'm not sure who could review this? I'm thinking maybe we could merge this unreviewed @patak-js

@patak-dev
Copy link
Member

I think we still didn't get approval for this one, maybe it can be discussed in our next team meeting 👍🏼

@richarddavenport
Copy link

I think we still didn't get approval for this one, maybe it can be discussed in our next team meeting 👍🏼

How'd the team meeting go?

@patak-dev
Copy link
Member

@richarddavenport we couldn't get to this one, we'll try to review it in the next one

@patrickleet
Copy link

Any update on a timeline for this being included? Is something still not working with it?

@patak-dev
Copy link
Member

@aleclarson would you clean up this one when you have some time? Looks like we never got to it at the end.

@avarun42
Copy link

any update here? would be great for this to go in

@aleclarson
Copy link
Member Author

I don't have the bandwidth to carry this forward.

If someone wants to finish what I started, go right ahead :)

@gsxdsm
Copy link

gsxdsm commented Feb 25, 2023

Thanks Alec - this is becoming more and more of a need especially as Sveltekit gains popularity. Where can we help in getting this merged in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

SSR SourceMap not loaded when debugging
8 participants