Skip to content
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

process: add source-map support to stack traces #29564

Closed
wants to merge 8 commits into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Sep 15, 2019

This uses SourceMap.js (shipped with V8) and the cache developed in #28960, to add source-map support to stack traces.

How it works:

If you run your program with the flag --source-map-support, and a source-map is found and cached for a file, stack traces are annotated with both the original and transpiled source positions:

Error: goodbye
    at  /Users/bencoe/oss/node-1/test/fixtures/source-map/inline-throw.min.js:1:43
         -> /Users/bencoe/oss/node-1/test/fixtures/source-map/throw.js:8:2
    at  Immediate.<anonymous> (/Users/bencoe/oss/node-1/test/fixtures/source-map/inline-throw.min.js:1:60)
         -> /Users/bencoe/oss/node-1/test/fixtures/source-map/throw.js:8:2
    at  processImmediate (internal/timers.js:439:21)

Providing both the original and transpiled source if possible.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
  • test with source-map generated by uglify.
  • test with source-map generated by babel.
  • test with source-map generated by TypeScript.
  • test with source-map generated by nyc/Istanbul.

This grows out of continuing conversation here.

And relies on #28960, which I would ideally like to land first. (landed).

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 15, 2019
lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
lib/internal/source_map/source_map.js Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
@benjamingr
Copy link
Member

Code LGTM with devsnek's comments addressed. I'll also play with this locally a bit and see if it behaves consistently. Thanks for this this is a big usability improvement in core for a lot of our users!

(Note this will probably have to be semver-major and with a CITGM run because it changes all errors and some libraries parse them)

@bcoe
Copy link
Contributor Author

bcoe commented Sep 15, 2019

@benjamingr do you think a major is necessary, even though someone will opt in with --experimental-source-maps, I'm thinking if we drop the experimental we'd probably just switch to --enable-source-maps and keep it opt in?

Open to other people's thinking on this.

@ljharb
Copy link
Member

ljharb commented Sep 15, 2019

While behind an experimental flag, I’d expect no changes to precipitate a major.

@devsnek devsnek added errors Issues and PRs related to JavaScript errors originated in Node.js core. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 15, 2019
@joyeecheung
Copy link
Member

If it's opt-in via flags I don't think this need to be semver-major. BTW I think it's fine to just use --enable-source-maps from the start with an experimental warning.

doc/api/cli.md Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
@benjamingr
Copy link
Member

Sorry for being unclear: I do not think this needs to be major with the flag but I do eventually think Node.js should turn this on by default and that change should be semver-major.

+1 for Joyee's idea of doing --enable-source-maps, I would even take this one step further because I'd like this to be opt-out eventually rather than opt-in and I'd prefer:

--source-maps=true which eventually would be the default and users would be able to opt out with --source-maps=false

@bcoe bcoe force-pushed the source-map-support branch 2 times, most recently from fe2171b to 3ee0717 Compare September 22, 2019 06:51
@bcoe
Copy link
Contributor Author

bcoe commented Sep 22, 2019

@LinusU I'm wondering if you have any feedback, as we start to build this out; I imagine you've bumped into some interesting edge cases building source-map-support.

@bcoe bcoe force-pushed the source-map-support branch 3 times, most recently from 8238a97 to ffddaf1 Compare September 28, 2019 20:21
@bcoe bcoe marked this pull request as ready for review September 28, 2019 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. source maps Issues and PRs related to source map support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants