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
Member

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.
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member Author

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 force-pushed the bcoe:source-map branch 2 times, most recently from fd155c3 to 3b07d5b Aug 19, 2019
@bcoe

This comment has been minimized.

Copy link
Member Author

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.

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved
src/inspector_profiler.cc Outdated Show resolved Hide resolved
src/inspector_profiler.cc 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/modules/cjs/loader.js Outdated Show resolved Hide resolved
src/inspector_profiler.cc Outdated Show resolved Hide resolved
@bcoe bcoe force-pushed the bcoe:source-map branch from 76d0c21 to a96165d Aug 24, 2019
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 force-pushed the bcoe:source-map branch from 18761ad to 4dab8e3 Sep 1, 2019
@bcoe bcoe changed the title sourcemap: initial SourceMap support process: initial SourceMap support 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++ 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
@addaleax addaleax referenced this pull request Sep 21, 2019
3 of 3 tasks complete
@bcoe bcoe force-pushed the bcoe:source-map branch from b90717a to 4329191 Sep 21, 2019
@nodejs nodejs deleted a comment from nodejs-github-bot Sep 21, 2019
@nodejs-github-bot

This comment has been minimized.

@bcoe bcoe force-pushed the bcoe:source-map branch from 4329191 to 2a6dcde Sep 21, 2019
@nodejs-github-bot

This comment has been minimized.

addaleax added 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

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

Landed in 8f06773 🎉

@addaleax addaleax closed this Sep 21, 2019
@bcoe bcoe deleted the bcoe:source-map branch Sep 22, 2019
targos added 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 referenced this pull request Sep 24, 2019
BridgeAR added 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
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
You can’t perform that action at this time.