Add support for 1:many source maps #91
Conversation
|
This PR Fixes #21 |
lib/v8-to-istanbul.js
Outdated
| // Upstream tooling can provide a block with the functionName | ||
| // (empty-report), this will result in a report that has all | ||
| // lines zeroed out. | ||
| if (block.functionName === '(empty-report)') { |
There was a problem hiding this comment.
this will no longer work properly.
I would remove it and make it an option.
|
@bcoe what do you think of the approach? I think I just have tests and loose ends to fix. |
|
@lukeapage sorry for the slow response, it's been a. busy few work weeks. I will make an effort to dig in soon and give you my opinion. |
| originalRawSource = await readFile(this.path, 'utf8') | ||
| try { | ||
| originalRawSource = await readFile(resolvedPath, 'utf8') | ||
| } catch(e) { |
There was a problem hiding this comment.
I needed to add this for me because my web app imported a package that imported a package and somewhere the source map path wasn’t made relative leading to failure.
You probably don’t want the console warn in - what are your preferences? An option to allow non found source files? Or an option to specify files to exclude?
There was a problem hiding this comment.
Specific file patterns to exclude would also improve performance by allowing excluding node modules.
There was a problem hiding this comment.
Ideally, I'd like to figure out a way to address the underlying bug, rather than suppressing an error ... I don't really want to introduce ignore logic in this layer, because there's already ignore logic the layer above in the c8; If it's not easy to address the root cause, because it's a 3rd party dependency, maybe the tool calling v8-to-istanbul could do a bit of filtering?
|
@bcoe no problem, I’m running off the github path so no hurry from me |
There was a problem hiding this comment.
I'm really excited about this work, but I'm finding it hard to give as thorough a review as I would like to due to a lack of tests (I tend to read tests first, and use this to figure out why implementation decisions were made).
I have some thoughts about testing (understanding that this repo can be a bit frustrating to write tests for).
Start with targeted unit tests
I would write tests specifically for SourceMapScannerIterator, and perhaps for a SourceFactory class if we go that route, instantiating those classes with fake data (rather than trying to perform an end to end test).
I do something along these lines here.
For end to end tests, use c8 to test
I've written a bunch of source-map integration tests based on real source-map producers for c8, you can use npm link to test your branch agains these:
cd v8-to-istanbul
npm link .
cd c8
npm link v8-to-istanbul
npm t
Jest also uses v8-to-istanbul, given it's popularity we should make sure that its tests continue functioning too.
Let's add additional tests to c8
It would be great to also add some real-world tests to c8 for a 1:many source map.
| return {} | ||
| } | ||
| const sourceMapIterator = sourceMapScanner.getIterator() | ||
| let start = sourceMapIterator.scanTo(lines[0].line, startCol - lines[0].startCol) |
There was a problem hiding this comment.
If I'm understanding the goal of this PR, it's to make it so that we can handle a source map for a library like uglify, which might translate one compiled file into many source files?
It feels like the logic for each individual source file should remain almost the same? I wonder if we could abstract this logic out a layer, such that the top level class has an array of CovSource objects produced by some sort of factory.
| return | ||
| } | ||
|
|
||
| let isEndLoc1BeforeLoc2Start = ((loc1.endLine < loc2.startLine) || (loc1.endLine === loc2.startLine && loc1.relEndCol < loc2.relStartCol)) |
There was a problem hiding this comment.
Shouldn't the SourceMap generator be making an effort to avoid overlapping source files? I wonder if we could add this logic later.
| @@ -146,17 +212,19 @@ function originalEndPositionFor (sourceMap, line, column) { | |||
| source: beforeEndMapping.source, | |||
| line: beforeEndMapping.line, | |||
| column: beforeEndMapping.column + 1, | |||
| bias: LEAST_UPPER_BOUND | |||
| bias: GREATEST_LOWER_BOUND | |||
There was a problem hiding this comment.
This logic has been thoroughly tested in c8, with several real world generators (take a look at the tests here).
I don't think we want to fiddle with this logic, as it grows out of a lot of testing.
| } | ||
|
|
||
| _binarySearchStart (startLine, startCol, startIndex, endIndex) { | ||
| if (startIndex === endIndex) { |
There was a problem hiding this comment.
I like the addition of this sourcemapScanner class.
This is kind of what I had in mind with regards to pulling the logic for creating multiple CovSource instances into some sort of factory, and trying to keep additional logic to a minimal in that file.
| originalRawSource = await readFile(this.path, 'utf8') | ||
| try { | ||
| originalRawSource = await readFile(resolvedPath, 'utf8') | ||
| } catch(e) { |
There was a problem hiding this comment.
Ideally, I'd like to figure out a way to address the underlying bug, rather than suppressing an error ... I don't really want to introduce ignore logic in this layer, because there's already ignore logic the layer above in the c8; If it's not easy to address the root cause, because it's a 3rd party dependency, maybe the tool calling v8-to-istanbul could do a bit of filtering?
|
Now we use |
|
Sorry but unfortunately the effect of covid-19 has been to reduce my free time (I have children) and I don’t know when I will get to this - I wrote it for another project that is also frozen so there’s nothing driving me to need to finish this off. |
|
Hi @lukeapage Thank you for developing this tool. It is really useful. Stay safe, stay healthy! |
This code seems to work on the repo I am trying to get it working on - that has typescript and then is compiled with webpack.
It would be cool to see if my changes produce better maps with people currently using this package.
- its a bit of a nightmare because there doesn't appear to be any automated formatting ie either prettier or eslint. It would make my life easier to put that in!mostly fixedI believe this is significantly less performant than the current version, because it doesn't assume that a chrome range start maps to the start in one file and the chrome range map end is the same as the end in another. In my project I found lots of occasions where this wasn't true, so I assume this will make single source map coverage mapping better too. I think we could use sourceMap.eachMapping instead of lots of queries and it would make things quicker. Its a shame that function doesn't allow you to set a start and end position, but probably even an immediate return for mappings outside the current range would work or else use it to populate our own sourcemap consumer that can get the nextOriginalMapping and endOfOriginalMapping etc.The new implementation is very fast. It could be a little faster by passing in an option to ignore files so that unnecessary computation isn't done on them