External source map is not supported

[BenceSzalai]

Problem

Dependency files that:

• include ESM dynamic imports
• has sourcemaps associated (generated by TSC)

show up in Error stack traces:

• with .js extension (as opposed to .ts)
• with wrong line numbers
• and with their real file path instead of the original path based on the sourcemap

i.e. their source maps are ignored.

Expected behavior

All dependency files that has sourcemaps associated should show up in Error stack traces with their original path, extension and line numbers.

Minimal reproduction URL

https://github.com/BenceSzalai/tsx-sm-issue

Version

v2.5.5

Node.js version

v18.17.0 / v20.5.1 (v16 is not affected)

Package manager

npm

Operating system

macOS

Contributions

• I plan to open a pull request for this issue
• I plan to make a financial contribution to this project

Notes

Originally opened against TXS, but it looks like it is an issue with ESM Loader.

[hanayashiki]

I think the similar problem exists if you require(index.js) and along side that index.js have a index.js.map.

It seems tsx should provide an option to distinguish running compiled .js v.s. running source code .js

[BenceSzalai]

Tbh I don't think that's related. I mean it sounds like a completely different thing. In the case this issue is about there is a sourcemap, and it is picked up and used by tsx except when the imported file has a dynamic import inside. Also i have attached a reproduction that clearly shows no relation to what you mentioned. Also you mention tsx should provide an option which seems like a feature request, while my case is something that is supposed to work without any new features; i.e. it is a bug.
Sorry, but these are more than likely different things. I'd recommend you to open separate case.

[privatenumber]

Might be related: #291 (Although, v18.7 should not be affected)

[BenceSzalai]

I've done some debugging and here's what's happening.

When we run the ts-wo-dyn-imp.ts there are two files to be transformed by esm-loader:

• ts-wo-dyn-imp.ts
• tsx-sm-package/ok.js

None of these contain dynamic imports, so the transformDynamicImport() function bails out here, so .source is not touched here, it just works fine.

However, when we run ts-w-dyn-imp.ts again there are two files to be transformed by esm-loader:

• ts-w-dyn-imp.ts - behaves same as above
• tsx-sm-package/notok.js - behaves different:

The file tsx-sm-package/notok.js contains a dynamic import, so transformDynamicImport() runs, so .source is overwritten here.

Now if we compare the contents of loaded.source before and after the change happening here we can see two differences:

• r.source is a Buffer before, but is a string after - seems to not affect Node
• r.source has a proper reference to the original sourcemap before, but has a duplicate sourcemap after - see below:

Before (result of toString() on the bufffer):

export async function foo() {
    throw new Error('x');
    // @ts-expect-error ignore the name of the imported package
    const bar = (await import('some-package')).default;
}
//# sourceMappingURL=notok.js.map

After:

export async function foo() {
    throw new Error('x');
    // @ts-expect-error ignore the name of the imported package
    const bar = (await import('some-package').then((mod)=>{const exports = Object.keys(mod);if(exports.length===1&&exports[0]==='default'&&mod.default.__esModule){return mod.default}return mod})).default;
}
//# sourceMappingURL=notok.js.map
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLC <<...truncated...>> DLENBQUMifQ==

The reason for the double //# sourceMappingURL... entry is that r.source is not the original TS file, but the JS file already compiled, which already has one source map associated, but esm-loader simply appends another sourcemap here.

Now this behaviour is byte-by-byte identical when running the reproduction code in Node v16.15 vs Node v20. So esm-loader does the same: adds a second sourcemap to an already "sourcemapped" file.

So after digging the changelogs of Node.js to find anything that warrants theis change in behaviour I've found this:
nodejs/node#44658

Basically what this PR does is to avoid picking up //# sourceMappingURL... magic strings in the middle of files. This was released in Node v18.10. So what we can see from this PR is that Node prior v18.10 was greedy looking for sourcemaps. Which in our case means that Node < v18.10 picked up //# sourceMappingURL=notok.js.map while Node v18.10 picks up //# sourceMappingURL=data:application/json;base64,...

And my assumption is that while the changes made to dynamic imports by esm-loader are within the same line, when Node is using the original sourcemap it simply causes a character position mapping issue in the line with the dynamic import, but technically it is still a valid sourcemap, although not exactly correct.

But, when Node started to pick the later sourcemap, that is now not mapping the file content with the rewritten dynamic import to the original TS file, but to the JS file already compiled. This matches exactly the issue I've found and reported here, only it looks like I did not understand the exact nature of the issue:

I thought that since the Error stack trace refers to a JS file instead of the original TS file sourcemaps were ignored. But what really happened is that Node did use the last sourcemap, only that one unfortunately at this point did not map the file to the original TS, but to the JS output from TSC. So after all the original sourcemap is ignored, but not because sourcemaps are ignored in general, but because it is shadowed by the sourcemap for the rewritten import.

As a sidenote let's acknowledge that multi level sourcemaps would be nice if worked, but they are not part of the specification and they do not work in any browser or runtime I know of.

So to solve the issue, and generally to make the sourcemapping right instead of simply appending a sourcemap here the following should be done:

• check if the source contains a //# sourceMappingURL... already.
• if so process that, meaning decode from base64 if inline or attempt to load from the filesystem if refers to a file.
• merge the original sourcemap and the sourcemap to be added.
• remove the original //# sourceMappingURL... from the source.
• append the new merged sourcemap.

Now merging sourcemaps is indeed not trivial, but also not impossible. There are tools and techniques for that. Some I've found by quick googling:

• https://www.npmjs.com/package/multi-stage-sourcemap
• https://www.npmjs.com/package/source-map-js with example here: sokra/source-map@add831d#diff-07a745dbe34cda6dccfa53a63cb8c551775bc93194d9d6225ee72659bcb59240R93

As an alternative to merging it might be possible to load the original sourcemap and use an API to make the import rewrites which would update those sourcemaps. I've seen similar thing using MagicString in Vite.js, but I couldn't find a documented way to make a new MagicString instance while loading existing Sourcemap, but SourceNode.fromStringWithSourceMap() of source-map-js may be able to do something similar as well.

Conclusion:
Source maps are not handled properly, but they used to be less broken in Node < v18.10, but now they are really broken.

[privatenumber]

Supplying source maps were never intentionally supported. This should be a feature request.

[BenceSzalai]

Okay, understood. With risking being a pain I'd still argue that just appending a new sourcemap at the end of a file which already has one "blindly" creates a broken mapping thus is a bug, although merging the existing and the new sourcemap would on the other hand really be an additional feature.

Probably a suitable way to fix the bug without putting too much effort into a new feature would be to not add additional sourcemap if there's already one. This suggestion is based on the assumption that whatever sourcemap is already included in the file is more relevant (most probably is the sourcemap for the compilation of TS to JS) than the sourcemap esm-loader would add, which only maps a small portion of the file and I think it would only map character positions without changing line numbers. If one is running a debugger the line numbers are crucial to match (needs the original sourcemap), while character positions within those very few lines with dynamic imports is mostly superficial.

Also I'm wondering, when you say merging the sourcemaps would be a feature request, would you consider such feature request to be in or out the scope for esm-loader?

[privatenumber]

In scope.

(Also, I don't mean to be rude but please keep your comments short and concise.)

[privatenumber]

I tried testing the latest tsx with this but the reproduction was actually not minimal and it was confusing to figure out.

Can you provide a StackBlitz reproduction with just one file with an inline source map, demonstrating it doesn't work with tsx?

[privatenumber]

Tests above confirms this has been working

[privatenumber]

🎉 This issue has been resolved in v4.11.1

If you appreciate this project, please consider supporting this project by sponsoring ❤️ 🙏