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

module: add API for interacting with source maps #31132

Closed
wants to merge 2 commits into from

Conversation

@bcoe
Copy link
Member

bcoe commented Dec 30, 2019

Background

When we introduced our own prepareStackTrace for rewriting stack traces based on source maps, we removed support users to provide a custom Error.prepareStackTrace...

V8 documents the Error.prepareStackTrace API surface and given feedback we've received, see #29994, it seems like this was the wrong design decision....

Unfortunately, since we don't expose the source map cache Node.js maintains, this means that the second someone overrides Error.prepareStackTrace, they lose the benefit of our source map handling...

first pass at fixing this...

In #29994 I proposed that we annotate call sites with the original source position, before calling Error.prepareStackTrace, as a way to provide tooling with a way to continue taking advantage of source maps... The concern was raised that we shouldn't extend the Error.prepareStackTrace further (since folks don't love this API surface).

What is this PR?

Rather than annotating the call sites passed to Error.prepareStackTrace, this PR proposes:

  1. returning Error.prepareStackTrace to its original behavior .
  2. exposing a public method for fetching source maps, source_map.findSourceMap(path[, error]), so that upstream tooling can still take advantage of source map handling, when overriding Error.prepareStackTrace.

I think this is a good approach, I'd intended on making the source map cache available to tooling eventually, and just hadn't had the time to do so... Addressing the issues in #29994, without completely removing source map support to upstream tooling, was motivation to do so.

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
@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Dec 30, 2019

@wesleytodd @dougwilson assuming that this PR gets some buy in, and we move towards an API we're happy with, I'd love to experiment with wiring this into express, so that we can make --enable-source-maps work with your overriding of Error.prepareStackTrace (this would be a good litmus test that we're getting the API to a healthy place).

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Dec 30, 2019

this is looking pretty good! would it be possible to separate re-enabling prepareStackTrace from the new features in a separate commit?

@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Dec 30, 2019

@devsnek yeah, I can do that, is the same PR okay, just split it into two commits? I'm about to board a plane so it will be tomorrow I think that I update.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Dec 30, 2019

@bcoe yeah same pr, and no rush 😄

@bcoe bcoe force-pushed the bcoe:source-map-public branch from 78b1c48 to 73542a7 Dec 30, 2019
<!--name=source_map-->

Exposes an API for interacting with Node.js' internal cache of source maps.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Dec 30, 2019

Member

If this is only useful for interacting with the internal cache, why do we need to expose the SourceMap constructor?

The source maps at the moment are essentially mapping between source positions, which are essentially tuples of (filename, line, column) or (filename, line, column, name) (target-only), why do we require users to pass an error to the API to get a source map first before fetching the mapping, instead of just exposing a straight-forward API with a signature like (filename, line, column) => {filename, line, column, name?} ? IIUC aren't the error-rekeying only useful for Module._load, which is written in JS and can't be source-mapped anyway?

This comment has been minimized.

Copy link
@bcoe

bcoe Dec 30, 2019

Author Member

If this is only useful for interacting with the internal cache, why do we need to expose the SourceMap constructor?

There are other use cases, and I thought it might be worthwhile to give developers access to the class:

  1. it means for their own unit testing, they can instantiate a SourceMap instance with a Source Map V3 payload; since we don't allow folks to directly access the cache, I think this would be beneficial.
  2. there are a few other methods I imagine we might expose, findEntryReversed, which was left out for now; methods for interacting with 1:many source maps (which potentially get more complex, and require more helpers); I think it might be an ugly abstraction putting these all on the cache.
  3. keep in mind the SourceMap instance is almost directly from V8, and compliant with the Source Map V3 specification; putting it in the hands of tooling authors will make it more likely that this feature sees adoption, I imagine folks might find use-cases I haven't thought of in Node.js' naive initial implementation of SourceMaps.
  • It came to mind too that source-map is one of the most downloaded dependencies on npm, and for simple use cases having the class SourceMap might do the trick for folks.

IIUC aren't the error-rekeying only useful for Module._load, which is written in JS and can't be source-mapped anyway?

Because of the WeakMap we ended up using to cache source maps against modules, keying on error is necessary if an exception happened while loading a module, so cases like this:

  Error.prepareStackTrace = (error, trace) => {
    const throwingRequireCallSite = trace[0];
    if (throwingRequireCallSite.getFileName().endsWith('typescript-throw.js')) {
      sourceMap = findSourceMap(throwingRequireCallSite.getFileName(), error);
      callSite = throwingRequireCallSite;
    }
  };
  try {
    // Require a file that throws an exception, and has a source map.
    require('../fixtures/source-map/typescript-throw.js');
  } catch (err) {
    err.stack; // Force prepareStackTrace() to be called.
  }

☝️ Given that one of the common use cases of source maps is prettifying stack traces, I think this will actually come up pretty often.

This comment has been minimized.

Copy link
@bcoe

bcoe Dec 30, 2019

Author Member

With regards to:

IIUC aren't the error-rekeying only useful for Module._load

I agree that it's not an intuitive API surface to ask the user to provide an error object, I wonder if it would be worth complicating the data-structure we store the source maps in a bit more (perhaps a secondary index?) to remove this burden from the user.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jan 3, 2020

Member

it means for their own unit testing, they can instantiate a SourceMap instance with a Source Map V3 payload; since we don't allow folks to directly access the cache, I think this would be beneficial.

How would they use this in tests? I don't think there is an API that takes a source map object and alters the internal cache? To me it's hard to imagine how one would write a test with this other than testing Node's own implementation of the source map parsing. If one wants to make sure that they emits the correct source map, I assume they would just write files to a temporary directory and check if they can get the correct positions from the Node after loading these files.

keep in mind the SourceMap instance is almost directly from V8

I am confused - what exactly comes from V8? The source maps are emitted by the users, we get the URI by parsing the file content with regex ourselves, and load them by reading from the file system / decoding the data URI ourselves? The SourceMap class is just a wrapper on top of the data we parse and read ourselves?

I think this will actually come up pretty often.

So error is only necessary when the source_map.findSourceMap may be invoked when handling an error that occurs during module loading? It still seems somewhat awkward to me that we require the user to do this due to the limitation of the current implementation, at least we should explain in the docs about the reason (and what would happen if they forget to pass it)

This comment has been minimized.

Copy link
@bcoe

bcoe Jan 5, 2020

Author Member

I am confused - what exactly comes from V8?

The class SourceMap is a faithful implementation of the Source Map Revision 3 Proposal, taken from the V8 codebase.

The source maps are emitted by the users, we get the URI by parsing the file content with regex ourselves, and load them by reading from the file system / decoding the data URI ourselves?

Many tooling authors have the use case of consuming source maps coming from a variety of locations, Node.js' cache I would expect to be just one...

As an example, take puppeteer which runs in the browser, but is orchestrated by Node.js; I would like the user to be able to take the SourceMaps fetched from code executed via the puppeteer harness, instantiate a SourceMap instance, and interact with it identically to as if they'd fetched it from Node.js' cache.

How would they use this in tests? I don't think there is an API that takes a source map object and alters the internal cache? To me it's hard to imagine how one would write a test with this other than testing Node's own implementation of the source map parsing

I was picturing writing a tool like Babel, that generates source-maps; if they want to write unit tests to ensure these payloads work properly with Node.js' implementation, because the payload format is standardized, they can simply instantiate a SourceMap class (there's no need for it to have ever been in Node.js' cache) -- this makes it easier to write targeted unit tests, without going through the dance of requiring a module in Node.js, and forcing it into cache.

So error is only necessary when the source_map.findSourceMap may be invoked when handling an error that occurs during module loading?

Correct, the error is required for exceptions that occur during the loading process; I'm happy to document this further.

I also think it's worth adding a few TODO comments, because we can get rid of this requirement as soon as WeakReferences are available in Node.js; I think the source-map cache can be simplified significantly with an interable WeakMap.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jan 6, 2020

Member

Thanks for the explanation.

taken from the V8 codebase.

Shouldn't the source of truth be https://cs.chromium.org/chromium/src/third_party/devtools-frontend/src/front_end/sdk/SourceMap.js ? It seems v8's copy has been outdated (also, it is only used in the tick processor, though TBH I have never use the tick processor with source maps before..)

This comment has been minimized.

Copy link
@bcoe

bcoe Jan 6, 2020

Author Member

@joyeecheung seems like I should read through the changes that have landed in front_end/sdk/SourceMap.js and bring any important ones over.

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Dec 30, 2019

I would prefer to split the prepareStackTrace re-enabling into a separate PR - that would be a semver-patch while this is a semver-minor and I think it's better to keep patches with different semver-ness in different PRs in general.

@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Dec 30, 2019

I would prefer to split the prepareStackTrace re-enabling into a separate PR - that would be a semver-patch while this is a semver-minor and I think it's better to keep patches with different semver-ness in different PRs in general.

The reason I didn't split this into two PRs is that they're fairly gated on one another, potentially the most common use case for looking up the source map, remapping the exceptional flow, won't work until Error.prepareStackTrace is reenabled.

So this PR ends up being gated on the other, if there's significant contention in this PR, I can open another, but it seemed like there were some benefits to doing the work together.

@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Dec 30, 2019

CC: @coreyfarrell the PR we were talking about on the weekend.

Copy link
Member

addaleax left a comment

Thank you for taking care of this!

@Trott
Trott approved these changes Dec 30, 2019
doc/api/source_map.md Outdated Show resolved Hide resolved
@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Dec 30, 2019

Given that both @devsnek and @joyeecheung asked this to be split into two PRs, I'm going to go ahead and do so, the Error.prepareStackTrace PR will be tiny, so shouldn't slow this down too much.

@bcoe bcoe mentioned this pull request Dec 31, 2019
3 of 3 tasks complete
@wesleytodd

This comment has been minimized.

Copy link
Member

wesleytodd commented Dec 31, 2019

For reference from above:

@wesleytodd @dougwilson assuming that this PR gets some buy in, and we move towards an API we're happy with, I'd love to experiment with wiring this into express

One of the deps of express is depd, which is one of @dougwilson's modules (not directly maintained as part of the express project): https://github.com/dougwilson/nodejs-depd

Copy link
Member

benjamingr left a comment

The barrier is high and I'm going to pretend the naming issues don't exist :D

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Jan 2, 2020

This is semver-major because of the new top-level module addition. Is there another way we can reasonably introduce this as a semver-minor instead? Such as exposing the new API off an existing top level module like util?


`error` if you are interacting with source maps in an exceptional flow, e.g.,
having overridden [`Error.prepareStackTrace(error, trace)`][], you should pass
the error instance as a second parameter to `findSourceMap`.

This comment has been minimized.

Copy link
@jasnell

jasnell Jan 2, 2020

Member

What's the expected behavior here when --enable-source-maps is not enabled.

This comment has been minimized.

Copy link
@bcoe

bcoe Jan 5, 2020

Author Member

If --enable-source-maps or NODE_V8_COVERAGE have not been set, then any call to findSourceMap will fail to find a source map corresponding to the path, and null will be the return value.

doc/api/source_map.md Outdated Show resolved Hide resolved
doc/api/source_map.md Outdated Show resolved Hide resolved
@bcoe bcoe force-pushed the bcoe:source-map-public branch 2 times, most recently from 2e3f169 to e548eae Jan 5, 2020
@bcoe bcoe added semver-minor and removed semver-major labels Jan 5, 2020
@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Jan 5, 2020

@joyeecheung @addaleax @jasnell @Trott @benjamingr take a look, I've moved the public API for source maps into the module namespace, which felt natural; I've also made an effort to address everyone's feedback.

@joyeecheung I hope I was able to express myself a bit better: the payload loaded by SourceMap is a standard, so I was picturing source-map producers like Babel can use it in unit testing (without forcing a module to load into our cache); also I think there's significant value in upstream tooling like puppeteer being able to use our SourceMap API (I expect this will drive adoption, and help iron out bugs).

@@ -112,6 +112,7 @@ class StringCharIterator {
* @param {SourceMapV3} payload
*/
class SourceMap {
#payload = null;

This comment has been minimized.

Copy link
@coreyfarrell

coreyfarrell Jan 5, 2020

Member

Why not leave this undefined if not set? Take function something(sourceMap = defaultSourceMap) {}, passing undefined to this function causes the default argument value to be used, passing null would not.

generatedLine: null,
generatedColumn: null,
originalSource: null,
originalLine: null,
originalColumn: null
Comment on lines 173 to 177

This comment has been minimized.

Copy link
@coreyfarrell

coreyfarrell Jan 5, 2020

Member

Explicit undefined for these values as well?

if (sourceMap && sourceMap.data) {
return new SourceMap(sourceMap.data);
} else {
return null;

This comment has been minimized.

Copy link
@coreyfarrell

coreyfarrell Jan 5, 2020

Member

Ditto on returning undefined.

lib/module.js Show resolved Hide resolved
* @return {Object} raw source map v3 payload.
*/
get payload() {
return this.#payload;

This comment has been minimized.

Copy link
@coreyfarrell

coreyfarrell Jan 5, 2020

Member

Should any steps be taken to protect this.#payload from manipulation?

sourceMapInstance.payload.sources = [];
@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented Jan 6, 2020

Still LGTM.

@jasnell jasnell dismissed their stale review Jan 6, 2020

updated.. need to re-review

doc/api/modules.md Outdated Show resolved Hide resolved
@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Jan 9, 2020

@joyeecheung functionality Chromium's source-map implementation looks close to the same, but the code has changed enough that I think it would make this review difficult, I've opened this tracking issue:

#31286

Would you be open to landing this PR, and I'll investigate moving us to Chromium's implementation in the future (the main advantage seems to be a few minor tweaks, such as sorting the source map ranges).

@jasnell how are you feeling about this PR?

bcoe added 2 commits Jan 5, 2020
@bcoe bcoe force-pushed the bcoe:source-map-public branch from e548eae to 3ea89d2 Jan 11, 2020
@nodejs-github-bot

This comment has been minimized.

@bcoe bcoe added the author ready label Jan 11, 2020
@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Jan 14, 2020

👋 @joyeecheung one last ping, wanted to make sure you were happy with where this landed, my intention being to come back and pull in any bug fixes from the chromium SourceMap implementation in an additional PR.

Mainly I just wanted to make sure we landed on a place where we're happy with the initial API.

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Jan 14, 2020

I am fine with this landing as an experimental API.

bcoe added a commit that referenced this pull request Jan 14, 2020
PR-URL: #31132
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Jan 14, 2020

Landed in 521b222

@bcoe bcoe closed this Jan 14, 2020
@bcoe bcoe deleted the bcoe:source-map-public branch Jan 14, 2020
MylesBorins added a commit that referenced this pull request Jan 16, 2020
PR-URL: #31132
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jan 16, 2020
codebytere added a commit that referenced this pull request Jan 20, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* doc:
  * add GeoffreyBooth to collaborators (Geoffrey Booth) #31306
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* doc:
  * add GeoffreyBooth to collaborators (Geoffrey Booth) #31306
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.