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: initial SourceMap support via NODE_V8_COVERAGE #28960

Closed
wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Aug 4, 2019

Adds support for caching source maps when they're observed in footer of script file, and outputting them to the coverage directory if NODE_V8_COVERAGE is enabled.

This is seen as an initial step towards more thorough support for source maps inside Node.js itself.

Goal of this PR

Get it to the point where upstream tooling can detect source maps, even in the cases where they've been inserted by custom loaders or require.extensions.

Future Work (outside the scope of this PR)

  1. expose source maps through an API, so that tooling can interact with them programmatically, e.g., fetching a SourceMap, or applying a SourceMap to a line/column position.
  2. apply source maps to Node.js' annotations of error objects.
  3. work with V8 folks towards adding source map support into V8 itself; with some of the functionality in Node.js (e.g., caching the source maps) and some of the functionality in V8 (e.g., having correct stack traces).
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

CC: @schuay, @snehasi (I've left some tests blank and I think this would be a great starting point for some of the work we're doing in the mentorship program).

Remaining Work
  • discuss future API direction (pull together design doc?).
    • I don't think I'll be able to write a design document up for a few weeks, but I think this initial implementation is a step in the right direction.
  • make sure collecting source-maps isn't a major performance regression for coverage.
  • add support for base64 encoded data URIs.

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 4, 2019
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be a WIP? Are we waiting for some V8 API addition that we can feed sourcemaps to to pair with this?

lib/internal/source_map.js Outdated Show resolved Hide resolved
test/fixtures/v8-coverage/sigint.js Outdated Show resolved Hide resolved
@bcoe
Copy link
Contributor Author

bcoe commented Aug 5, 2019

Is this supposed to be a WIP? Are we waiting for some V8 API addition that we can feed sourcemaps to to pair with this?

@joyeecheung I've been thinking about this design problem for a little bit of time now, and the conclusion I've been coming to is we'd need to support source maps in both Node.js and V8:

Role I feel V8 should serve in supporting source maps

  1. it would be nice if the stack traces thrown from V8 applied source maps (perhaps it includes both the transpiled position and original position, if a script has been created with a source map).
  2. it would potentially be nice if source maps were automatically applied to the coverage iniformation output by V8 (this would simplify writing upstream tooling ... my only concern is that it's perhaps a lot of engineering for the value we'd get, if tooling could instead be applied a layer above).

Role I feel Node.js should serve in supporting source maps

  1. it should detect source maps in a script being executed: loading the file if it's on disk; decoding data-URLs from inline source maps.
  2. Node.js should pass the decoded source map to the script created in V8, so that it can be applied appropriately to stack traces.
  3. Assuming V8 doesn't apply source maps to coverage generated (at least for some time) it would be nice if we could output source map information at exit, similarly to how we output coverage information (this would allow tooling to take into account source maps inserted at run time).
  4. It would be nice to maintain a cache of the source maps loaded for files, and to expose this as an API, so that upstream tooling could interact with the source maps we've extracted for scripts.
  5. Node.js performs post processing on the errors returned by V8, even if V8 applies source maps, it would be nice to be able to also correct this error output.

The scope of this proposal

I propose we start simple, by collecting source maps from source files, caching them, and exposing them similarly to how we expose coverage.

In follow up pull requests we could then gradually introduce additional behavior:

  1. rewriting stack traces (which we could shim in Node.js until it's supported in V8).
  2. exposing the source map cache as an API.

@bcoe bcoe added the wip Issues and PRs that are still a work in progress. label Aug 13, 2019
@bcoe bcoe force-pushed the source-map branch 2 times, most recently from fd155c3 to 3b07d5b Compare August 19, 2019 05:16
@bcoe
Copy link
Contributor Author

bcoe commented Aug 19, 2019

@joyeecheung l added the method WriteSourceMapCache, which writes the source map cache to disk at the same time that the coverage data is written.

To me, it seemed like the most straight forward place to expose source maps from their JavaScript implementation in source_map.js to inspector_profiler.cc for serializing, was as an accessor on the global process object.

As you've recommended, I intend to extend on the source-map support further to:

  1. load source-maps from file paths.
  2. parse base64 encoded data-urls.
  3. load source maps from other contexts, such as ESM modules.

Which is why I'd like to keep much of the implementation in source_map.js, taking advantage of higher level APIs, ideally just performing serialization in C++, if we can find an approach that makes folks happy.

src/inspector_profiler.cc Outdated Show resolved Hide resolved
src/inspector_profiler.cc Outdated Show resolved Hide resolved
@Trott Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 20, 2019
lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved
lib/internal/source_map.js Outdated Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
src/inspector_profiler.cc Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved
lib/internal/source_map.js Outdated Show resolved Hide resolved
lib/internal/source_map.js Show resolved Hide resolved
@nodejs nodejs deleted a comment from nodejs-github-bot Aug 26, 2019
@bcoe bcoe changed the title sourcemap: initial SourceMap support process: initial SourceMap support Sep 2, 2019
@bcoe bcoe added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Sep 2, 2019
@bcoe bcoe changed the title process: initial SourceMap support process: initial SourceMap support tied to NODE_V8_COVERAGE Sep 2, 2019
@bcoe bcoe added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 2, 2019
@bcoe bcoe changed the title process: initial SourceMap support tied to NODE_V8_COVERAGE process: initial SourceMap support via NODE_V8_COVERAGE Sep 2, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Sep 2, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Sep 2, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Sep 2, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Sep 20, 2019
@nodejs nodejs deleted a comment from Freya716 Sep 20, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Sep 20, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Sep 20, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Sep 21, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Sep 21, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Sep 21, 2019
PR-URL: #28960
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@addaleax
Copy link
Member

Landed in 8f06773 🎉

@addaleax addaleax closed this Sep 21, 2019
@bcoe bcoe deleted the source-map branch September 22, 2019 18:02
targos pushed a commit that referenced this pull request Sep 23, 2019
PR-URL: #28960
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
BridgeAR added a commit that referenced this pull request Sep 24, 2019
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
PR-URL: #28960
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
BridgeAR added a commit that referenced this pull request Sep 25, 2019
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512

PR-URL: #29695
BridgeAR added a commit that referenced this pull request Sep 25, 2019
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512

PR-URL: #29695
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. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.