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.coffeeCoffeeScript.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

+ifprocess.version.split(".")[0] !="v12"
+test"native source maps", ->
+newPromise (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 <[email protected]>

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

+ifprocess.version.split(".")[0] !="v12"
+test"native source maps", ->
+newPromise (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 <[email protected]>

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

Fixesjashkenas#5382
4d9b901

build:browser
0fb2951

GeoffreyBooth reviewed Apr 1, 2022
View reviewed changes

src/coffeescript.coffee Outdated Show resolvedHide 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 85cc164Compare 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 c942bd0Compare April 3, 2022 22:23

Added guard for in browser
264b009

GeoffreyBooth reviewed Apr 3, 2022
View reviewed changes

src/coffeescript.coffee Outdated Show resolvedHide resolved

GeoffreyBooth reviewed Apr 3, 2022
View reviewed changes

STRd6and others added 2 commits April 3, 2022 18:41

Use --require rather than -r
9f65df3
Co-authored-by: Geoffrey Booth <[email protected]>

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 resolvedHide 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

STRd6and others added 6 commits April 17, 2022 10:28

attachStackTrace -> patchStackTrace
58afee8

Apply suggestions from code review
8e2f090
Co-authored-by: Geoffrey Booth <[email protected]>

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 resolvedHide resolved
test/sourcemap.coffeeShow resolvedHide resolved
test/sourcemap.coffeeShow resolvedHide 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:mainApr 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

		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

Uh oh!

Conversation

STRd6 commented Apr 1, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

Uh oh!

STRd6 commented Apr 1, 2022

Uh oh!

STRd6 commented Apr 1, 2022

Uh oh!

STRd6 commented Apr 1, 2022

Uh oh!

GeoffreyBooth commented Apr 3, 2022

Uh oh!

Uh oh!

Uh oh!

GeoffreyBoothApr 3, 2022

Choose a reason for hiding this comment

Uh oh!

GeoffreyBoothApr 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

STRd6 commented Apr 4, 2022

Uh oh!

STRd6 commented Apr 4, 2022

Uh oh!

GeoffreyBooth commented Apr 7, 2022

Uh oh!

STRd6 commented Apr 8, 2022

Uh oh!

Uh oh!

STRd6 commented Apr 11, 2022

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

GeoffreyBoothApr 17, 2022

Choose a reason for hiding this comment

Uh oh!

STRd6Apr 17, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeoffreyBoothApr 18, 2022

Choose a reason for hiding this comment

Uh oh!

STRd6Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GeoffreyBoothApr 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

STRd6 commented Apr 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

STRd6 commented Apr 18, 2022

Uh oh!

GeoffreyBooth commented Apr 18, 2022

Uh oh!

STRd6 commented Apr 18, 2022

Uh oh!

GeoffreyBooth commented Apr 18, 2022

Uh oh!

STRd6 commented Apr 18, 2022

Uh oh!

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

STRd6Apr 17, 2022•
edited
Loading