feat(vite): full source-map support for ssr

[DylanPiercey]

Description

This PR implements my suggestion from and resolves #3288.
With this change SSR modules transformed via ssrLoadModule now receive an inline sourceMappingURL as a datauri.

This means with --enable-source-maps or using --require source-map-support/register with node debugging, errors and whatnot are all source mapped properly.

Additional context

One performance related detail about this implementation is that I've set it up to avoid serializing sourcesContent when there is already a file on disk by checking for mod.file. If there is no file then it will inline the sources content.

Also because this change should invalidate the need for ViteDevServer.ssrFixStacktrace I have deprecated it in this PR. We could however keep it around if anyone thinks there is any value there.

---

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.

[Shinigami92]

Seems you are not on the latest main and 60 commits behind
Could you update your branch?

You can do this nowadays with GitHub Web UI!
Just visit your fork and use the Fetch and merge button

[DylanPiercey]

@Shinigami92 I've now rebased and added the return type a suggested.
Let me know if anything else needs to change here 😄

[brillout]

I tried this PR against vite-plugin-ssr's test suite. They fail because the following piece of code doesn't work anymore.

// Check whether `val` is a plain JavaScript object
function isPlainObject(val) {
  return val && val.constructor === Object;
}

The vm.runInNewContext seems to create a second Object value.

I couldn't find an alternative way to implement isPlainObject(). I could dig deeper and maybe find a solution, but this is a (slight) problem for vite-plugin-ssr.

If I remove all isPlainObject() instances, then all tests go green though.

The --enable-source-maps flag is still Stability: 1 - Experimental. Not being able to trace errors is one of the most painful experience; I'm not sure we want to be experimental here?

Also, forcing my users to use --enable-source-maps is not ideal. I'm wondering if that would also apply to SvelteKit users: would they need to svelkit dev --enable-source-maps? Once Node.js native Source Map support stabilizes, they'll probably then remove the flag.

It would be super neat to get rid of ssrFixStackTrace, but maybe we should wait for Node.js's Source Map support to stabilize?

[DylanPiercey]

@brillout probably this should be changed to not rely on runInNewContext, my main reason for going that route was to avoid using new Function (source map inconsistency) or manually appending the wrapper code (requires augmenting the sourcemap). But yeah this doesn't really work and will have edge cases as you mention. I'll try to find some time to augment this to probably use my latter suggestion.

Regarding --enable-source-maps, it is no longer experimental (https://nodejs.org/dist/latest-v16.x/docs/api/cli.html#cli_enable_source_maps) but does still require being set. The alternative, which I mention in the deprecation message, is to use a tool like https://www.npmjs.com/package/source-map-support. One thing we could do to smooth things out for the user it to detect if sourcemaps have been loaded via --enable-source-map and fall back to that module if not. Curious what you all think of that option? It would involve evaling some psuedo sourcemapped code that throws an error and checking to see if the stack was mapped or not.

[brillout]

@DylanPiercey Neat digging 👍.

I'm just seeing:

Overriding Error.prepareStackTrace prevents --enable-source-maps from modifying the stack trace.

How would we allow Vite users to override Error.prepareStackTrace while also allowing them to ssrFixStacktrace?

it is no longer experimental

Do we know whether there are breaking changes between v12 and v16? Quite relevant since we need to support all versions since 12+.

Also, forcing my users to use --enable-source-maps is not ideal. I'm wondering if that would also apply to SvelteKit users: would they need to svelkit dev --enable-source-maps? Once Node.js native Source Map support stabilizes, they'll probably then remove the flag.

Thoughts on this? IMO, forcing all users to use a flag is less than ideal.

Does Node.js plan to set the flag to true by default? @vdeturckheim

use a tool like https://www.npmjs.com/package/source-map-support

I don't know how source-map-support works; seems like it does magic that could lead to conflicts?

[brillout]

Actually, why don't we use Error.prepareStackTrace instead of --enable-source-map?

[DylanPiercey]

@brillout that would only solve part of the issue. The main issue from my perspective is that there is no way currently to hook up a debugger with proper source maps when using vite for ssr.

[vdeturckheim]

Does Node.js plan to set the flag to true by default? @vdeturckheim

I don't know but it would sound weird to me to enable such behaviour by default in node.
But doing --require X is usually the equivalent of running require('X') as the first line of code of the app.
Alternatively, it might be possible to dig in the codebase to see if the --enable-source-maps flag could be set at runtime but that might have some potential security impact I am not fully certain of yet.

I don't know Vite enough to think of alternatives that could fit the desired UX.

[brillout]

I opened a feature request to replace --enable-source-maps with --disable-source-maps nodejs/node#38817.

[aleclarson]

Closing in favor of #3928