fix: dont overwrite sources array #120
Conversation
mattlewis92
force-pushed
the
master
branch
3 times, most recently
from
4888252
to
c1b6ed4
Compare
Current coverage is 98.42% (diff: 100%)@@ master #120 diff @@
==========================================
Files 15 15
Lines 573 573
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 564 564
Misses 9 9
Partials 0 0
|
|
Sorry for the delay, I will take a look at this this week. |
|
@jdonaghue do you have an eta on 0.9.0? |
|
@dangler My goal is to have us land the 3 open PRs this month and release 0.9 this month. |
|
Thank you! |
|
apologies for not getting to this last week. I am going to look at this first thing tomorrow. |
|
@jdonaghue please review asap, this is our last remaining PR to consider for 0.9.0 which I'd like to release this week. Thanks! |
|
@dylans I have reviewed this already this morning. Sorry, for the delay! Just waiting on an feedback/update from @mattlewis92 |
| @@ -123,7 +123,7 @@ export default class CoverageTransformer { | |||
| if (sourceMap.sourcesContent) { | |||
| origSourceFilename = rawSourceMap.sources[0]; | |||
|
|
|||
| if (origSourceFilename && path.extname(origSourceFilename) !== '') { | |||
| if (origSourceFilename && path.extname(origSourceFilename) !== '' && rawSourceMap.sources.length === 1) { | |||
| origFileName = rawSourceMap.file; | |||
There was a problem hiding this comment.
@mattlewis92 This looks good, but if rawSourceMap.sources contains more than one those sources will never run through this code. I am thinking it might be better to put lines 128-131 into a loop looping over rawSourceMap.sources? What are your thoughts?
There was a problem hiding this comment.
I did initially try that but it didn't fix the original issue. This was the only solution I could find that worked.
The idea behind this hackish fix is that currently if there are > 1 sources this block will always throw that error and make remap-istanbul unusable. The ideal solution would be to make a proper test case, and fix the underlying issue, however I don't have the knowledge myself to be able to do that 😞
There was a problem hiding this comment.
@jdonaghue any further thoughts on this?
There was a problem hiding this comment.
@dylans @mattlewis92 My only concern is that if there were multiple sources this code would not process sources at all in that scenario. However, this code does address the original issue and puts it back more-or-less at parity with prior to the change that caused that issue in the first place (in terms of this particular section of code). I am OK with landing this and then we can assess whether or not we need to look at it further. I will merge it right now.
Thanks for your work @mattlewis92 !
This PR: #65 introduced a bug that would overwrite the sources array and lead to issues like this: marcules/karma-remap-istanbul#31
My workaround is to only apply this fix if there is only 1 source - however this is probably completely the wrong way to resolve this - any guidance would be greatly appreciated 😃