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 lineLength to source-map-cache #29863

Closed
wants to merge 4 commits into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Oct 6, 2019

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

Problem

I realized when integrating source-map functionality with coverage that we're missing a piece of information necessary to support tools like ts-node:

  • V8's coverage implementation is based on absolute byte offset, whereas Source Map V3 is based on line/column offset.
  • when code is transpiled in memory (using hooks like require.extensions) we lack enough information to translate from bytes to lines/columns.

we end up with a bad reports that look like this, due to missing information:

Screen Shot 2019-10-06 at 11 25 03 AM

👆 note that exactly the wrong code is highlighted 😆

Solution

If we track the line lengths of the source file that source maps were extracted from, this provides enough information to remap from byte offset to line/column offset.

I've introduced an additional value into the cache, lineLengths, to facilitate this.

we can now generate the following report:

Screen Shot 2019-10-06 at 11 28 07 AM

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 6, 2019

const debug = require('internal/util/debuglog').debuglog('source_map');
const { findSourceMap } = require('internal/source_map/source_map_cache');
const { overrideStackTrace } = require('internal/errors');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled prepare_stack_trace.js into its own file, because it seemed like it wasn't directly related to the source map caching functionality (and source_map_cache.js was starting to get a bit confusing).

@@ -38,18 +37,22 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) {

const match = content.match(/\/[*/]#\s+sourceMappingURL=(?<sourceMappingURL>[^\s]+)/);
if (match) {
const data = dataFromUrl(basePath, match.groups.sourceMappingURL);
const url = data ? null : match.groups.sourceMappingURL;
Copy link
Contributor Author

@bcoe bcoe Oct 6, 2019

Choose a reason for hiding this comment

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

It's getting to the point where we write quite a bit of data to disk (and memory) -- we don't actually need the url field, if we've successfully parsed the map, so let's avoid storing it.

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

i can't really comment on the changes to the source map logic but everything else looks ok

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@bcoe bcoe added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 7, 2019
@bcoe bcoe removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 9, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Oct 11, 2019

@targos I believe I've addressed your comments, mind giving this a final look?

@nodejs-github-bot

This comment has been minimized.

Without the line lengths of in-memory transpiled source, it's not
possible to convert from byte ofsets to line/column offsets.
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

Trott pushed a commit that referenced this pull request Oct 14, 2019
Without the line lengths of in-memory transpiled source, it's not
possible to convert from byte ofsets to line/column offsets.

PR-URL: #29863
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 14, 2019

Landed in 4ca61f4

@addaleax addaleax added the source maps Issues and PRs related to source map support. label Nov 2, 2019
targos pushed a commit that referenced this pull request Nov 8, 2019
Without the line lengths of in-memory transpiled source, it's not
possible to convert from byte ofsets to line/column offsets.

PR-URL: #29863
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
Without the line lengths of in-memory transpiled source, it's not
possible to convert from byte ofsets to line/column offsets.

PR-URL: #29863
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. 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