Bug: Requiring "coffeescript" breaks Node.js built-in source map support globally

[jaens]

Node.js has built in support for source maps via the --enable-source-maps command-line option.
If any library at any point requires "coffeescript", that support gets completely broken (ie. Node.js stack traces stop processing source maps for eg. TypeScript).

I am guessing this is due to the entry point in coffee-script.js unconditionally overriding Error.prepareStackTrace.

Input Code

test.ts, compiled to test.js (with sourcemaps).
package.json has "type": "module" (probably irrelevant).

import 'coffeescript';

throw new Error("test");

Run with node --enable-source-maps test.js.

Expected Behavior

Stack trace refers to test.ts.

Current Behavior

Stack trace refers to test.js.

Context

One of the libraries I use requires "coffeescript" and now all my stack traces are broken.

Environment

• CoffeeScript version: 2.6.0
• Node.js version: 16.10

[GeoffreyBooth]

The 1.x line is no longer developed. Does this occur with 2.x?

[jaens]

The 1.x line is no longer developed. Does this occur with 2.x?

Yes, upgraded to 2.6.0, same result.

[jaens]

I am guessing overriding Error.prepareStackTrace also breaks the commonly used source-map-support library (assuming coffeescript is loaded after it).

The following comment also seems inaccurate:

coffeescript/src/coffeescript.coffee

Lines 365 to 368 in d572d74

	# NodeJS / V8 have no support for transforming positions in stack traces using
	# sourceMap, so we must monkey-patch Error to display CoffeeScript source
	# positions.
	Error.prepareStackTrace = (err, stack) ->

...considering that there are now at least 2 standard solutions for source maps in Node.js.

[GeoffreyBooth]

We could wrap that Error.prepareStackTrace monkey-patching in an unless process.execArgv.includes('--enable-source-maps') to only patch when the --enable-source-maps flag wasn’t passed.

[jaens]

We could wrap that Error.prepareStackTrace monkey-patching in an unless process.execArgv.includes('--enable-source-maps') to only patch when the --enable-source-maps flag wasn’t passed.

Options can also be passed via the NODE_OPTIONS environment variable, not sure if that shows up in execArgv.
Also, that would not unbreak the source-map-support library...

[GeoffreyBooth]

Options can also be passed via the NODE_OPTIONS environment variable, not sure if that shows up in execArgv.
Also, that would not unbreak the source-map-support library…

Well, you could do unless process.execArgv.includes('--enable-source-maps') or process.env.NODE_OPTIONS.includes('--enable-source-maps'). I think those are the only ways to pass arguments to the Node process.

How would this not unbreak source-map-support? That library doesn’t have coffeescript as a dependency (other than for its own tests, but unless you’re developing source-map-support itself you won’t load its development dependencies).

[jaens]

How would this not unbreak source-map-support? That library doesn’t have coffeescript as a dependency (other than for its own tests, but unless you’re developing source-map-support itself you won’t load its development dependencies).

Because that library works via handling Error.prepareStackTrace, so if anything requires coffeescript after requiring source-map-support (quite likely, since source-map-support tends to be required very early), it would just overwrite it.

[jaens]

Personally, I can't see how unconditionally overwriting globals would ever always work nicely with other languages/libraries/features, I think it would be best to refer users to either the Node.js option or the centralized source-map-support library if they would like to have proper stack traces, instead of the ad-hoc patching that only works if the entire app only uses CoffeeScript.
(the CS-only monkeypatch could still be supported, just optional instead of always-on)

[GeoffreyBooth]

The monkey-patch is very old code. Changing it at this point might break other projects that are relying on it. See #4428 for a glimpse of the history.

I think scoping the monkey-patch to not happen when Node is run via --enable-source-maps is a safe first step; the user is explicitly asking Node to handle source maps in that case, and CoffeeScript should respect that. We could go further and put the monkey-patch inside coffeescript/register, so that it’s only applied when register is used, but I’m not sure what side effects on other projects such a change might cause.

[jaens]

The monkey-patch is very old code. Changing it at this point might break other projects that are relying on it. See #4428 for a glimpse of the history.

Yeah, though I guess the question is if stack trace breakage would warrant a new major version or not, since it probably does not affect actual functionality in the majority of cases. (also considering that if users already are upgrading, there are better stackmap processing options now that they should also switch to anyway)

I think scoping the monkey-patch to not happen when Node is run via --enable-source-maps is a safe first step; the user is explicitly asking Node to handle source maps in that case, and CoffeeScript should respect that. We could go further and put the monkey-patch inside coffeescript/register, so that it’s only applied when register is used, but I’m not sure what side effects on other projects such a change might cause.

Yeah, certainly that is better than nothing. When going down the route of workarounds, another one that could be added is not overriding Error.prepareStackTrace when it has already been set. (it should normally be undefined)

[STRd6]

There is a similar but related issue with source-map-support breaking CoffeeScript source maps if it is required after CoffeeScript. A change to both libraries could get them to work together where the patched Error.prepareStackTrace function exposes a .maps property where any library could add mappings.

So if Error.prepareStackTrace exists and has a .maps property then don't patch it and stuff the source maps into its .maps. If Error.prepareStackTrace is undefined we're free to patch it. If it is defined but doesn't have .maps then replacing it may adversely affecting another library.

All this may be moot if --enable-source-maps or source-map-support are the more mature recommended ways of handling this. In that case making sure that CoffeeScript works correctly with those tools would be a better way to spend the effort.

[GeoffreyBooth]

I think Node’s --enable-source-maps is the preferred way at this point, and no projects should be monkey-patching anymore. CoffeeScript should be updated so that we don’t apply the patch if Node’s --enable-source-maps is turned on.