Add source maps to node loader

[remcohaszing]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and couldn’t find anything (or linked relevant results below)
• If applicable, I’ve added docs and tests

Description of changes

This allows people to debug MDX files when using the Node.js loader. I confirmed this works using the VSCode debugger. I’m not sure how to test this properly.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mdx	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 4, 2024 2:19pm

[wooorm]

How would people use this? What has a VS Code debugger to do with running node and its loaders from a command line?

Can we detect if some source maps flag is passed to Node? Then it can work the same as Rollup: auto-add a SourceMapGenerator to options.

You can change the test script in the package.json file to include such a flag.

Or, can this be default behavior?

[remcohaszing]

How would people use this? What has a VS Code debugger to do with running node and its loaders from a command line?

People use this by debugging a script that imports MDX. For example by specifying the --enable-source-maps Node.js option or by using the JavaScript Debug Terminal from VSCode.

I was using @mdx-js/node-loader to test if breakpoints from VSCode work in MDX files. They almost automagically do.

Can we detect if some source maps flag is passed to Node? Then it can work the same as Rollup: auto-add a SourceMapGenerator to options.

I like this idea. Given the following script:

import inspector from 'node:inspector'

console.dir(process.sourceMapsEnabled)
console.dir(process.execArgv)
console.dir(inspector.url())

Running with node --enable-source-maps script.js yields:

true
[ '--enable-source-maps' ]
undefined

Running from the VSCode JavaScript Debug Terminal yields:

false
[]
'ws://127.0.0.1:36027/d594f5d2-a3dd-4f1e-8484-7ed291f5b7b3'

Note that process.sourceMapsEnabled is still experimental.

You can change the test script in the package.json file to include such a flag.

👍

Or, can this be default behavior?

You mean always pass SourceMapGenerator? Maybe. I don’t know if this has any performance impact. On the other hand, if people really care about performance, they should just build their project before deploying to production instead of using a Node.js loader. It’s always useful to have stack traces mapped to their original location.

[wooorm]

Running from the VSCode JavaScript Debug Terminal yields:

Wait, does that mean that auto-detecting this would not work with your debugger? 🤔
Or, does it work if you run that debugger with a certain flag?
If we’re going with “automatically works”, then the debugger should be supported too? Or is that a mistake on the VS Code debugger’s part?

Or do you mean to enable this feature when either inspector.url() is truthy or process.sourceMapsEnabled is on?

You can change the test script in the package.json file to include such a flag.

Perhaps we should spawn different Node processes in the tests btw. That way we can properly test whether (in the future, different) flags are passed?

I don’t know if this has any performance impact.

Yeah, that’s the only potential downside, right?
I’d assume generating the source map is fast. It’s just some positional info in a comment at the bottom.
The lookup part might be slow, but stack traces are slow anyway, and folks would pass that flag if they want that.

[remcohaszing]

Running from the VSCode JavaScript Debug Terminal yields:

Wait, does that mean that auto-detecting this would not work with your debugger? 🤔 Or, does it work if you run that debugger with a certain flag? If we’re going with “automatically works”, then the debugger should be supported too? Or is that a mistake on the VS Code debugger’s part?

Or do you mean to enable this feature when either inspector.url() is truthy or process.sourceMapsEnabled is on?

It basically means there are two distinct features that benefit from source maps.

1. --enable-source-maps, detectable via process.sourceMapsEnables, for stack traces.
2. Node.js debug inspector, detectable via inspector.url(), for debugging.

You can change the test script in the package.json file to include such a flag.

Perhaps we should spawn different Node processes in the tests btw. That way we can properly test whether (in the future, different) flags are passed?

Yes, even better.

I don’t know if this has any performance impact.

Yeah, that’s the only potential downside, right? I’d assume generating the source map is fast. It’s just some positional info in a comment at the bottom. The lookup part might be slow, but stack traces are slow anyway, and folks would pass that flag if they want that.

Yes, that’s the only potential downside. I’m fine with adding SourceMapGenerator by default in @mdx-js/node-loader. Should we allow overriding it by explicitly passing undefined?

---

Two more things I noticed:

1. The esbuild integration doesn’t support source maps. Supporting source maps for esbuild appears to work the same: just add the sourceMappingURL comment, based on their Svelte plugin example.
2. Source maps are slightly off, by 1 line, or a statement, or a few bytes. I’m not entirely sure.

[wooorm]

Are you planning to continue working on this, or were you waiting for a response from me?

[remcohaszing]

Just this question: Should we always pass SourceMapGenerator in @mdx-js/node?

[wooorm]

Yeah 👍

[wooorm]

Are you looking into the Windows failure?

Also, the different spawned threads? (“Perhaps we should spawn different Node processes in the tests btw.”)

[remcohaszing]

Are you looking into the Windows failure?

Ugh, Windows path separators must be one of the most expensive mistakes in the history of software.

Also, the different spawned threads? (“Perhaps we should spawn different Node processes in the tests btw.”)

This was useful to test detect whether source maps are useful as discussed earlier. Since we now just always enable source map generation, we no longer have to test this detection.