-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support source maps #1862
Support source maps #1862
Conversation
I am not sure why the CI test on Node v14 failed. Everything passed on v10 and v12 (and locally on v14 as well).
Seems like a rounding error to me? Is there any way to re-run the CI? |
Can you tell me how to reproduce the problem that this solves? I regularly use Jasmine in Node to test TypeScript code and I haven't seen a non-source-mapped stack trace in literally years, so I think there's probably an important difference between your setup and mine. |
Hmm, that's interesting. I just use the normal TypeScript compiler. No other tools involved (no webpack, etc.) I have created a minimal example that shows the problematic behaviour: minimal-example.zip Just run these three commands in the folder:
You should see output that contains the non-sourcemapped stack trace:
I am curious about your setup as well. It would be great if there was a way to get it working without modifying Jasmine (as long as I don't need any extra npm dependencies... Trying to keep it minimal). Node.js version: 15.0.1 |
Thanks, that helps. I'm able to reproduce what you're seeing. I usually use The approach you've taken here looks fine to me. If you'll add a test for this (probably next to, and in the same style as, the existing stack trace filtering tests) and modify the source files in |
The failure you saw on Node 14 is a flaky test that I haven't found the time to deal with. Don't worry about it. |
Thanks, that sounds great! I have followed your advice. Could you please review again? In the test I used Windows-style paths with backslashes and starting with The rest should be in the same style as the existing tests. |
Sorry, I originally forgot to revert the changes to |
I just squashed my commits into a single commit to make the history a bit cleaner |
Description
Node.js >=12.12.0 has support for source maps when run with the flag
--enable-source-maps
but the additional stack trace information is not being shown by Jasmine's stack traces. This pull request stops stack trace information from being discarded. Among other things, this causes the source map information to show up.Motivation and Context
Currently Jasmine discards source map information from stack traces when the
file
property is not set on those stack frames.This would proably fix #491.
It might be relevant that the "competing" framework Mocha has specific support for
--enable-source-maps
: https://mochajs.org/#-enable-source-mapsPersonally, I would prefer a completely unmodified stack trace from Node/V8, because I think that most users are educated enough to be able to read those. The extra information is just too valuable.
How Has This Been Tested?
I have manually tested this with a test suite that throws an error. Before running Jasmine I had to set the following environment variable:
The stack trace looks much more complete now and it finally includes the source map information. I am not sure if it's feasible to add an automated test, because the feature requires at least Node.js v12.12.0, so the test would fail on Node.js 10. Additionally, we would have to
--enable-source-maps
for the test process.Output before:
Output after:
Note that this includes the correct TypeScript filename as well as correct line/column numbers. Many editors (such as VS Code) can turn this into a clickable link that directly opens the source file.
Types of changes
Checklist: