Don’t patch `Error.prepareStackTrace` if `--enable-source-maps` is used. #5403

STRd6 · 2022-04-01T18:32:43Z

Fixes #5382 Don't monkey patch Error.prepareStackTrace if --enable-source-maps is used.

This ended up being kind of a chonker but I'm pretty happy with the overall structure. Also a little terrified with how much this touches, as well as a little relieved that there are tons of existing tests to catch regressions 😅

In summary:

Calling require('coffeescript') no longer patches Error.prepareStackTrace. People must require('coffeescript/register') to opt-in. CoffeeScript.run already does this which will maintain the existing coffee cli experience.
Error.prepareStackTrace is not patched if another library has already patched it even when calling require('coffeescript/register').
Move Error.prepareStackTrace patching in coffeescript.coffee is now exported rather than applied automatically. register.coffee imports it. The browser docs also call it directly.
Moved caching of source maps into sourcemap.coffee (not sure if this is the ideal place).
Tracking anonymously compiled modules by unique number rather than the brittle array walking of all files under the same <anonymous> name.
- This makes getSourceMap and registerCompiled much simpler.
- Still exporting registerCompiled so other tools can cache source maps and hook into CoffeeScript stack remapping (CoffeeCoverage, etc.)
- Needed to change helpers.syntaxErrorToString to print [stdin] for <anonymous.* files as well as files with undefined file name to match the existing tests.
In index.coffee CoffeeScript.run set inlineMap to be true and set the base option.filename to match the mainModule filename.

TODO

CoffeeScript.compile always caches source maps if present. This makes the call to registerCompiled in register.coffee redundant. Need to do some thinking about how to untangle this.
Test case for ./bin/coffee --nodejs --enable-source-maps test/importing/error.coffee (Looks like file path is currently incorrect: C:\Users\duder\Documents\GitHub\coffeescript\test\..test\importing\error.coffee:3:9)
Browser testing. I believe that modern browsers correctly handle inline source maps but it would be a good idea to have tests for the specific cases and to make sure we're adding them where appropriate. We should always add inline source maps when running / executing scripts and should respect the options given when used as a tool for compiling.

src/coffeescript.coffee

STRd6 · 2022-04-01T19:14:50Z

It looks like node 16.14.x breaks the dynamic import test which worked in 16.13.x. Not sure what the correct resolution for that is.

STRd6 · 2022-04-01T19:15:16Z

Tests also pass on node 17.8.0

STRd6 · 2022-04-01T21:27:44Z

Node 16.14.x tests are fixed and updated here: #5404

GeoffreyBooth · 2022-04-03T03:05:51Z

Node 16.14.x tests are fixed and updated here: #5404

If you don’t mind updating with the latest main, this should be good to land now.

- Added check to not patch `Error.prepareStackTrace` if it has already been patched by another library. - Always attach inline source maps if `--enable-source-map` is set. - Added some basic tests for stack trace.

…urate...

src/coffeescript.coffee

test/sourcemap.coffee

GeoffreyBooth · 2022-04-03T23:25:29Z

test/sourcemap.coffee

+if process.version.split(".")[0] != "v12"
+  test "native source maps", ->
+    new Promise (resolve, reject) ->
+      proc = spawn "node", [


You can use child_process.fork to specifically spawn Node child processes: https://nodejs.org/api/child_process.html#:~:text=The%20child_process.fork()%20method%20is%20a%20special%20case%20of%20child_process.spawn()%20used%20specifically%20to%20spawn%20new%20Node.js%20processes.

Is there a reason you used fork in the above test but spawn here?

test/sourcemap.coffee

Co-authored-by: Geoffrey Booth <456802+GeoffreyBooth@users.noreply.github.com>

STRd6 · 2022-04-04T02:39:47Z

I'm pretty happy with how this is functionally. There's some more clean up and testing that can be followed up with but I'm fairly confident that this will maintain the existing functionality while fixing the two cases where patching Error.prepareStackTrace breaks things. We now will no longer patch if Error.prepareStackTrace has been changed by another tool (such as source-map-support) and we also no longer patch if --enable-source-maps is set since that would apply an additional remapping making the stack trace incorrect there as well. The default behavior is still to patch Error.prepareStackTrace so that the basic 'out of the box' experience for people using CoffeeScript will still be correct stack traces.

STRd6 · 2022-04-04T13:55:18Z

There's actually one other case that this doesn't address correctly yet: when --enable-source-maps is present in code that calls CoffeeScript.compile (think webpack or a similar bundler) then as it stands right now it will always add an inline map even if that option wasn't specified to the compile function.

The fix is to move the logic into register and pass the correct options to compile based on the environment settings. I'll continue working in that direction. The downside is that it is a larger change overall but it should end up with simpler code that is clearer and more direct which will also clean up some of the historical weirdness around source maps.

`path.relative` doesn't include a trailing separator which causes source maps to have an inaccurate file path when using `--enable-source-maps` in node.

GeoffreyBooth · 2022-04-07T16:19:39Z

Please let me know when you’re done working on this branch and I’ll review it again. Please don’t forget the style notes (single-quoted strings unless interpolating, avoid single-character variables).

STRd6 · 2022-04-08T23:32:11Z

I'll get a few more updates in and summarize the changes as well over the next few days.

src/stacktrace.coffee

STRd6 · 2022-04-11T18:03:38Z

@GeoffreyBooth this is ready for review now. It's a bit larger than I originally anticipated. Please ask any questions you have so I can better document exactly what changed and how things work.

I'm not sure why the CI is flaky on windows but I should look into that before this is merged.

GeoffreyBooth

This is very good! A few notes here and there and this should be ready to land.

Does this fix #5216 by chance? From a quick search of open issues, that appears to be the only other open issue related to source maps.

GeoffreyBooth · 2022-04-17T04:28:01Z

src/helpers.coffee

+exports.anonymousFileName = do ->
+  n = 0
+  ->
+    "<anonymous-#{n++}>"


Should this use square brackets (like [anonymous-0]) to match [stdin]?

It was previously <anonymous> which matches the "filename" of Node's eval.

which matches the “filename” of Node’s eval.

That’s fine; do you mind sharing where you see that in Node?

node Welcome to Node.js v17.9.0. Type ".help" for more information. > eval("throw new Error") Uncaught Error at eval (eval at <anonymous> (REPL1:1:1), <anonymous>:1:7) >

src/coffeescript.coffee

test/sourcemap.coffee

GeoffreyBooth · 2022-04-17T05:55:55Z

test/sourcemap.coffee

+if process.version.split(".")[0] != "v12"
+  test "native source maps", ->
+    new Promise (resolve, reject) ->
+      proc = spawn "node", [


Is there a reason you used fork in the above test but spawn here?

test/sourcemap.coffee

Co-authored-by: Geoffrey Booth <456802+GeoffreyBooth@users.noreply.github.com>

STRd6 · 2022-04-17T20:59:47Z

Does this fix #5216 by chance? From a quick search of open issues, that appears to be the only other open issue related to source maps.

It doesn't fix #5216 but I think it lays some groundwork that could lead to a fix.

test/sourcemap.coffee

STRd6 · 2022-04-18T22:04:02Z

One thing to note is that though this does fix a bug/unexpected behavior it could be considered a breaking change to how require('coffeescript') works. The good news is that it is very simple to restore the previous behavior if people want by calling patchStackTrace explicitly.

CoffeeScript = require('coffeescript')
CoffeeScript.patchStackTrace()

GeoffreyBooth · 2022-04-18T22:49:55Z

The good news is that it is very simple to restore the previous behavior if people want by calling patchStackTrace explicitly.

I thought the intent was to keep the current behavior unless --enable-source-maps was passed (or another plugin already called Error.prepareStackTrace).

STRd6 · 2022-04-18T23:16:55Z

I think that the correct long term solution is to not do anything drastic to people's environments when require('coffeescript') is called, similarly to how require.extensions patching was moved to /register. Requiring coffeescript/register still patches as before (excepting the cases when --enable-source-maps is provided or Error.prepareStackTrace has already been set) but requiring coffeescript does not as currently implemented in this PR.

If that is too breaking a change to go out soon then I can update the PR to also have coffeescript continue to patch until the next breaking release for compatibility.

GeoffreyBooth · 2022-04-18T23:25:29Z

I thought the current behavior was not to patch unless register was called; are we patching when just requireing?

If so, yeah, we should restrict the patching to register (and within register, don’t patch if --enable-source-maps is set).

STRd6 · 2022-04-18T23:40:37Z

👍

The current behavior in 2.6.1 patches no matter what when required:

coffeescript/src/coffeescript.coffee

Line 368 in 76cf769

Error.prepareStackTrace = (err, stack) ->

This PR restricts the patching to register and doesn't patch in those cases mentioned. People can use the CoffeeScript.patchStackTrace() if they need to for compatibility reasons.

GeoffreyBooth · 2022-04-19T03:01:24Z

src/coffeescript.coffee

+    sourceURL = "//# sourceURL=#{filename}"
    js = "#{js}\n#{sourceMapDataURI}\n#{sourceURL}"


FYI, not as part of this PR but related: #5237

GeoffreyBooth · 2022-04-19T03:11:41Z

Thanks @STRd6 and congrats! We’re overdue for a release, are there any other PRs you’d like to land soon or should I prepare a release for later this week?

STRd6 · 2022-04-19T03:25:49Z

@GeoffreyBooth thanks for the review! I don't have anything else ready to land so release when ready 👍

STRd6 added 2 commits April 1, 2022 11:28

Fixes jashkenas#5382

4d9b901

build:browser

0fb2951

GeoffreyBooth reviewed Apr 1, 2022

View reviewed changes

src/coffeescript.coffee Outdated Show resolved Hide resolved

GeoffreyBooth changed the title ~~Fixes #5382~~ Don’t patch Error.prepareStackTrace if --enable-source-maps is used. Apr 3, 2022

STRd6 added 2 commits April 3, 2022 10:25

Merge remote-tracking branch 'origin/main' into 5382

bebcec0

Basic tests and inline source maps when native source maps are enabled

9354f97

- Added check to not patch `Error.prepareStackTrace` if it has already been patched by another library. - Always attach inline source maps if `--enable-source-map` is set. - Added some basic tests for stack trace.

STRd6 force-pushed the 5382 branch 2 times, most recently from 50dfa8e to 85cc164 Compare April 3, 2022 22:10

Added exemption for node v12, native source maps don't seem to be acc…

c942bd0

…urate...

STRd6 force-pushed the 5382 branch from 85cc164 to c942bd0 Compare April 3, 2022 22:23

Added guard for in browser

264b009

GeoffreyBooth reviewed Apr 3, 2022

View reviewed changes

src/coffeescript.coffee Outdated Show resolved Hide resolved

GeoffreyBooth reviewed Apr 3, 2022

View reviewed changes

STRd6 and others added 2 commits April 3, 2022 18:41

Use --require rather than -r

9f65df3

Co-authored-by: Geoffrey Booth <456802+GeoffreyBooth@users.noreply.github.com>

PR Feedback

05a9502

STRd6 added 9 commits April 4, 2022 11:26

Experimental big refactoring of source mappings

6c1312b

Better tracking anonymous files

3cbbf48

Comment cleanup

b71bad6

Restored NODE_OPTIONS environment variable check

dad126e

Added test for NODE_OPTIONS

44234dd

Node v12 ... my old nemesis

946a82f

Simplified

fae524b

Added default options object

f971278

Ensuring relative path for sourceRoot ends with separator

46a3962

`path.relative` doesn't include a trailing separator which causes source maps to have an inaccurate file path when using `--enable-source-maps` in node.

STRd6 added 2 commits April 10, 2022 15:20

Single quote string style

1049cc6

Fixed stack trace in browser docs

4015fc5

GeoffreyBooth reviewed Apr 11, 2022

View reviewed changes

src/stacktrace.coffee Outdated Show resolved Hide resolved

STRd6 added 2 commits April 11, 2022 10:12

Removed new stacktrace.coffee file

8590d28

Don't display inline maps on doc examples

e6ab9f6

GeoffreyBooth reviewed Apr 17, 2022

View reviewed changes

STRd6 and others added 6 commits April 17, 2022 10:28

attachStackTrace -> patchStackTrace

58afee8

Apply suggestions from code review

8e2f090

Co-authored-by: Geoffrey Booth <456802+GeoffreyBooth@users.noreply.github.com>

Coding style cleanup

2467d7d

Updated comment on registerCompiled export

219f76a

doc:test

267d91c

Fixed test guard conditional

d24c75b

GeoffreyBooth reviewed Apr 17, 2022

View reviewed changes

test/sourcemap.coffee Outdated Show resolved Hide resolved

test/sourcemap.coffee Show resolved Hide resolved

test/sourcemap.coffee Show resolved Hide resolved

STRd6 added 2 commits April 17, 2022 16:47

Slightly more robust version checking

0d64fcc

Comment about spawn vs fork

c56d9a2

GeoffreyBooth reviewed Apr 19, 2022

View reviewed changes

GeoffreyBooth merged commit 8d32c71 into jashkenas:main Apr 19, 2022

This was referenced Apr 19, 2022

Add patchStackTrace export; test that all named exports are detected by Node #5411

Merged

2.7.0 #5412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don’t patch `Error.prepareStackTrace` if `--enable-source-maps` is used. #5403

Don’t patch `Error.prepareStackTrace` if `--enable-source-maps` is used. #5403

STRd6 commented Apr 1, 2022 •

edited

Loading

STRd6 commented Apr 1, 2022

STRd6 commented Apr 1, 2022

STRd6 commented Apr 1, 2022

GeoffreyBooth commented Apr 3, 2022

GeoffreyBooth Apr 3, 2022

GeoffreyBooth Apr 17, 2022

STRd6 commented Apr 4, 2022

STRd6 commented Apr 4, 2022

GeoffreyBooth commented Apr 7, 2022

STRd6 commented Apr 8, 2022

STRd6 commented Apr 11, 2022

GeoffreyBooth left a comment

GeoffreyBooth Apr 17, 2022

STRd6 Apr 17, 2022 •

edited

Loading

GeoffreyBooth Apr 18, 2022

STRd6 Apr 18, 2022

GeoffreyBooth Apr 17, 2022

STRd6 commented Apr 17, 2022

STRd6 commented Apr 18, 2022

GeoffreyBooth commented Apr 18, 2022

STRd6 commented Apr 18, 2022

GeoffreyBooth commented Apr 18, 2022

STRd6 commented Apr 18, 2022

GeoffreyBooth Apr 19, 2022

GeoffreyBooth commented Apr 19, 2022

STRd6 commented Apr 19, 2022

		sourceURL = "//# sourceURL=#{filename}"
		js = "#{js}\n#{sourceMapDataURI}\n#{sourceURL}"

Don’t patch Error.prepareStackTrace if --enable-source-maps is used. #5403

Don’t patch Error.prepareStackTrace if --enable-source-maps is used. #5403

Conversation

STRd6 commented Apr 1, 2022 • edited Loading

TODO

STRd6 commented Apr 1, 2022

STRd6 commented Apr 1, 2022

STRd6 commented Apr 1, 2022

GeoffreyBooth commented Apr 3, 2022

GeoffreyBooth Apr 3, 2022

Choose a reason for hiding this comment

GeoffreyBooth Apr 17, 2022

Choose a reason for hiding this comment

STRd6 commented Apr 4, 2022

STRd6 commented Apr 4, 2022

GeoffreyBooth commented Apr 7, 2022

STRd6 commented Apr 8, 2022

STRd6 commented Apr 11, 2022

GeoffreyBooth left a comment

Choose a reason for hiding this comment

GeoffreyBooth Apr 17, 2022

Choose a reason for hiding this comment

STRd6 Apr 17, 2022 • edited Loading

Choose a reason for hiding this comment

GeoffreyBooth Apr 18, 2022

Choose a reason for hiding this comment

STRd6 Apr 18, 2022

Choose a reason for hiding this comment

GeoffreyBooth Apr 17, 2022

Choose a reason for hiding this comment

STRd6 commented Apr 17, 2022

STRd6 commented Apr 18, 2022

GeoffreyBooth commented Apr 18, 2022

STRd6 commented Apr 18, 2022

GeoffreyBooth commented Apr 18, 2022

STRd6 commented Apr 18, 2022

GeoffreyBooth Apr 19, 2022

Choose a reason for hiding this comment

GeoffreyBooth commented Apr 19, 2022

STRd6 commented Apr 19, 2022

Don’t patch `Error.prepareStackTrace` if `--enable-source-maps` is used. #5403

Don’t patch `Error.prepareStackTrace` if `--enable-source-maps` is used. #5403

STRd6 commented Apr 1, 2022 •

edited

Loading

STRd6 Apr 17, 2022 •

edited

Loading