Risk of accidentally registering both source-map-support and @cspotcode/source-map-support

[ejose19]

Search Terms

source map
10.2.0

Expected Behavior

Log should point to example.ts:3

Actual Behavior

Log points to example.ts:4

Steps to reproduce the problem

• Clone the repro repository
• Run bash run.sh

Minimal reproduction

TypeStrong/ts-node-repros#17

Specifications

• ts-node version: 10.2.0+
• node version: 14.17.0 / 16.6.1
• TypeScript version: 4.3.5 / 4.4.2
• tsconfig.json, if you're using one:

{
  "compilerOptions": {
    "target": "ESNEXT",
    "module": "CommonJS",
    "strict": true,
    "moduleResolution": "node",
    "esModuleInterop": true,
    "skipLibCheck": true,
    "forceConsistentCasingInFileNames": true
  }
}

• Operating system and version: Archlinux

Additional context

I see that on https://github.com/TypeStrong/ts-node/releases/tag/v10.2.0 there was some changes to source map, probably causing this issue.

EDIT:

After reading #1440 & #1438, I see that this is issue is present due to using 2 different source-map-support libraries, however in this case it's a dependency (tslog) using the original source-map-support, how should users proceed in this case? I've confirmed that if I change the import in the dependency to require("@cspotcode/source-map-support"), it points to the correct line number, but that's not a practical solution.

[cspotcode]

Removing source-map-support should fix the issue, since ts-node already installs equivalent functionality via @cspotcode/source-amp-support. Alternatively, switching to @cspotcode/source-map-support, since it is a fork of the original with improved performance, bugfixes, and features.

We may also brainstorm ways to detect when both source-map-support libraries are loaded and somehow make them collaborate rather than interfere. Can we detect when source-map-support attempts to hook into node and we've already done so, or vice versa?

Another option is for contributors to work with the source-map-support maintainers to merge all of the bugfixes and improvements from @cspotcode/source-map-support. Once that is done, we can switch back to using source-map-support.

[cspotcode]

Another idea:

We send a PR to source-map-support allowing interop between source-map-support implementations. When a library attempts to .install() source-map-support, it can detect when an alternative source-map-support implementation has already been loaded and can instead pass the options along to the already-loaded implementation.

We could also send a PR to tslog asking them to switch to @cspotcode/source-map-support.

[ejose19]

Removing source-map-support nor switching to the fork is an option there, as it's not a dependency of the project, but a dependency of a dependency (tslog).

Also, seeing that https://github.com/evanw/node-source-map-support/commits/master last commit was almost 1y ago, the merge of the bug fixes doesn't seem like a solution in the short term.

So if this could be handled by @cspotcode/source-map-support (like stated in your 2nd point) then it would be a shorter term solution while the original library has their bugs fixed.

[cspotcode]

I took another look at source-map-support's hooking of node. It does 3x things:

Sets Error.prepareStackTrace

https://github.com/evanw/node-source-map-support/blob/d0587453869fa19f35671f32f4e27514123877a7/source-map-support.js#L560-L564

If we install our hook first, we can use Object.defineProperty and use a heuristic to detect their prepareStackTrace implementation. When recognized, we can silently ignore it.

monkey-patches process.emit

https://github.com/evanw/node-source-map-support/blob/d0587453869fa19f35671f32f4e27514123877a7/source-map-support.js#L482-L497

I think we can use the same trick as above.

optionally monkey-patches require('module').prototype._compile

https://github.com/evanw/node-source-map-support/blob/d0587453869fa19f35671f32f4e27514123877a7/source-map-support.js#L537-L552

I don't think we need to prevent this last one. It is used to cache the source code strings from in-memory transpilers, which can later be used by the prepareStackTrace hook. But without the prepareStackTrace hook, doesn't seem like it causes any side-effects.

[cspotcode]

I suppose one problem with this approach:
If a library passes options to require('source-map-support').install(/* options here */) then those options will be silently ignored since we'll be blocking their hooks. I'm not sure if there is some way we can get access to those options.

[cspotcode]

Since we already hook require(), we could detect when source-map-support is loaded and rewrite it so that we can intercept calls to install() but that seems WAY too hacky for my taste.

All the same, perhaps it is an acceptable solution? We are already forced to do some pretty hacky things.

[ejose19]

Yes, I was playing around with how to redirect the subsequent require() of source-map-support to @cspotcode/source-map-support, since your fork made backward compatible changes, it seems like a good option.

[cspotcode]

Oh right, redirecting the require.resolve, interesting.

[cspotcode]

I want to log a warning when this happens, because that'll put pressure on maintainers to either fix source-map-support or to switch to @cspotcode/source-map-support. But then users are caught in the cross-fire, being forced to file bug reports and to enable a flag that disables the warnings.

[cspotcode]

I renamed this issue to more closely match the discussion. I've been trying to do that more often so that issues more clearly describe the work.

[ejose19]

Sure, I was about to change it as well. Got it working with the redirect, will open a PR on @cspotcode/source-map-support.