fix: ensure inputSourceMap is a plain object before instrumenting

[JLHwung]

This PR fixes a regression introduced in #484. When lib-instrument converts coverageData to AST nodes, babel will throw on the inputSourceMap because Babel don't know how to represent this object as literals since inputSourceMap is an instance of SourceMapGenerator.

In this PR I ensure inputSourceMap is a plain object and restore it after coverageData has been converted to AST. I am not familiar with the whole picture so please correct me if I overlooked any mistakes.

Context:
babel/babel#11614
jestjs/jest#10089

[JLHwung]

It could be a performance kill if the inputSourceMap is big. @coreyfarrell WDYT? Are we going to print inputSourceMap to the AST?

[coreyfarrell]

@JLHwung sorry it's taken me so long to respond. We do print inputSourceMap to AST, this is used to remap coverage data to the original source. Source mapping is very difficult and we've had (continue to have) many issues with people running mismatched transpile settings so we save the input source map to the specific coverage data.

I'm a bit confused as coverageData.inputSourceMap should always be a plain object. Do you know how this is not the case? My preference would be to fix the bug which allows inputSourceMap to not be a plain object. In theory we could have inputSourceMap just be a string to reduce AST memory overhead. That would be a breaking change though, maybe something we could consider when babel 8 is available (the plan is for istanbuljs next majors upgrade to babel 8).

[JLHwung]

coverageData.inputSourceMap should always be a plain object

I agree. But since inputSourceMap is in our API, I suggest we should validate if it is a plain object before it is consumed later, and finally thrown when instrumenting, which could be quite confusing: At that time we have lost the reason of why t.valueToNode is passed with a non-plain object.

vue-jest used to pass a SourceMapGenerator instance instead of a plain json object (fixed in vuejs/vue-jest#275). This PR was created then. I am good with closing this PR and instead we validate inputSourceMap in next major.

[coreyfarrell]

I'm unclear are you suggesting babel 8 will validate inputSourceMap or that istanbul-lib-instrument should validate it?

[JLHwung]

I think it would be nice for istanbul-lib-instrument to validate it since Babel is not the only downstream users. But yeah Babel 8 could validate it, too.

[coreyfarrell]

Sorry for the slow response. I don't object to adding a check if I got a PR for it. I think we should probably just throw an error in that situation (this should be non-breaking since it will only effect code that is already broken?). I don't think istanbul-lib-instrument should be doing JSON.parse / JSON.stringify to fix broken input.

One thing I should mention my plan is for the next semver-major of babel-plugin-istanbul not use istanbul-lib-instrument, istanbul-lib-instrument will use babel-plugin-istanbul. The branch I'm working on is https://github.com/istanbuljs/babel-plugin-istanbul/tree/next. Still have to experiment with migrating istanbul-lib-instrument to use this updated branch, I'm sure I'll find some minor issues with the current babel plugin when I do that.

[sargonpiraev]

Can we merge this?
I have checked that this work for me with the same problem

[coreyfarrell]

Can we merge this?
I have checked that this work for me with the same problem

I don't think I'm considering merging this as is. In my opinion if something that is not a plain object is given to istanbul that is a bug from whatever created those invalid options. I'd be OK with validating and directly throwing if the wrong object is provided but I don't like the idea of performing heaving processing (JSON stringify + parse) to work around an external bug.

@JLHwung do you know if babel 8 is actually adding any validation of this option (rejection of invalid input)?

[marekdedic]

Hi,
I just spent half a day debugging leading to this issue, so in case anyone finds this in the future, both babel-plugin-istanbul, coverage-istanbul-loader are susceptible to this issue - they pass the source map (which in my case was a SourceMap, not a POJO) straight into Instrumenter.instrument...

[bcoe]

@JLHwung don't suppose you'd be up for dusting this off, could we use the approach @marekdedic has taken with object spread, rather than JSON.parse?

Another potential optimization would be for us to check if it's a POJO, and only perform the coercion if necessary, I think we could get away with something like?

if (Object.getPrototypeOf(arg) !== Object.prototype) = sourceMap = {...sourceMap}

[marekdedic]

If you (the maintainers of istanbul) would consider merging this, I think I could update the PR pretty easily... However, as per previous comments by @coreyfarrell I though this needed to be done on the other side, so 🤷 ...

[bcoe]

@marekdedic I'd be comfortable with a patch that:

1. console.warns so that upstream implementers passing in the wrong object type correct the issue.
2. checks the prototype, so that we're not always coercing large objects (for an edge case).

[bcoe]

Thanks for the work on this @JLHwung, I've merged the alternate PR that adds tests.