Add support for 1:many source maps #91

lukeapage · 2020-02-13T20:36:41Z

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.

Formatting - there are mistakes ~~- 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 fixed
Tests - many of the tests assume implementation details so these fail
Performance - I 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

lukeapage · 2020-02-13T20:37:06Z

This PR Fixes #21

lib/source.js

lib/v8-to-istanbul.js

lukeapage · 2020-02-13T20:42:47Z

lib/v8-to-istanbul.js

+            // 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)') {


this will no longer work properly.
I would remove it and make it an option.

lukeapage · 2020-02-16T19:40:19Z

@bcoe what do you think of the approach? I think I just have tests and loose ends to fix.
It would be good to run this against some existing projects and compare the coverage to see if its improved, since I've changed the way sourcemapping is done.

bcoe · 2020-03-01T03:07:18Z

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

lukeapage · 2020-03-01T06:23:23Z

lib/v8-to-istanbul.js

-          originalRawSource = await readFile(this.path, 'utf8')
+          try {
+            originalRawSource = await readFile(resolvedPath, 'utf8')
+          } catch(e) {


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?

Specific file patterns to exclude would also improve performance by allowing excluding node modules.

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?

lukeapage · 2020-03-01T06:25:02Z

@bcoe no problem, I’m running off the github path so no hurry from me

bcoe

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.

bcoe · 2020-03-07T22:02:19Z

lib/source.js

-      return {}
-    }
+    const sourceMapIterator = sourceMapScanner.getIterator()
+    let start = sourceMapIterator.scanTo(lines[0].line, startCol - lines[0].startCol)


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.

bcoe · 2020-03-07T22:03:52Z

lib/source.js

+            return
+          }
+
+          let isEndLoc1BeforeLoc2Start = ((loc1.endLine < loc2.startLine) || (loc1.endLine === loc2.startLine && loc1.relEndCol < loc2.relStartCol))


Shouldn't the SourceMap generator be making an effort to avoid overlapping source files? I wonder if we could add this logic later.

bcoe · 2020-03-07T22:06:52Z

lib/source.js

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


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.

bcoe · 2020-03-07T22:10:19Z

lib/sourcemapScanner.js

+    }
+
+    _binarySearchStart (startLine, startCol, startIndex, endIndex) {
+        if (startIndex === endIndex) {


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.

bcoe · 2020-03-07T22:14:08Z

lib/v8-to-istanbul.js

-          originalRawSource = await readFile(this.path, 'utf8')
+          try {
+            originalRawSource = await readFile(resolvedPath, 'utf8')
+          } catch(e) {


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?

link89 · 2020-05-28T00:51:06Z

Now we use v8-to-Istanbul with istanbul-lib-source-maps to solve this issue.
How do you think to use an existed solution instead of implementing a new one from scratch?

lukeapage · 2020-05-28T03:10:25Z

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.

link89 · 2020-05-31T17:51:33Z

Hi @lukeapage Thank you for developing this tool. It is really useful. Stay safe, stay healthy!

lukeapage added 3 commits February 12, 2020 21:31

wip

c169da3

junk

8a42d95

tidy up

5864058