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

fix: make --all work for transpiled code #1047

Merged
merged 2 commits into from
Apr 3, 2019
Merged

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Apr 3, 2019

during normal operation nyc will instantiate SourceMaps multiple times:

  • once during initial instrumentation/execution step.
  • once when reporting takes place.

Because --all does not save source-maps to cache (as is the case when files are loaded during normal execution) the source-maps extracted by --all are dropped on the floor and never used.

This tiny fix makes source-maps start working in conjunction with --all.

@bcoe bcoe requested review from coreyfarrell and JaKXz April 3, 2019 05:30
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 96.356% when pulling c2c3408 on source-map-cache into b7e16cd on master.

8 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 96.356% when pulling c2c3408 on source-map-cache into b7e16cd on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 96.356% when pulling c2c3408 on source-map-cache into b7e16cd on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 96.356% when pulling c2c3408 on source-map-cache into b7e16cd on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 96.356% when pulling c2c3408 on source-map-cache into b7e16cd on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 96.356% when pulling c2c3408 on source-map-cache into b7e16cd on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 96.356% when pulling c2c3408 on source-map-cache into b7e16cd on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 96.356% when pulling c2c3408 on source-map-cache into b7e16cd on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 96.356% when pulling c2c3408 on source-map-cache into b7e16cd on master.

@coveralls
Copy link

coveralls commented Apr 3, 2019

Coverage Status

Coverage remained the same at 96.349% when pulling 1f4e0b0 on source-map-cache into b7e16cd on master.

@coreyfarrell
Copy link
Member

Does this resolves any of the issues where --all doesn't work at all on pre-instrumented sources? If so can we get a test to demonstrate non-covered pre-instrumented files being reported?

@@ -4,12 +4,12 @@ const libSourceMaps = require('istanbul-lib-source-maps')
const fs = require('fs')
const path = require('path')

// TODO: write some unit tests for this class.
let sourceMapCache = libSourceMaps.createSourceMapStore()
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about how this can have any effective change. The SourceMaps object is only created by the NYC object as nyc.sourceMaps. Since each process only gets one object, the singleton doesn't actually get reused by multiple SourceMaps objects. My vote is that we don't use singletons if it can be avoided.

Above aside if this is kept as a singleton variable I think it should be const, not let.

Copy link
Member Author

Choose a reason for hiding this comment

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

this should be a const.

The issue is that we new NYC() during instrumentation, and then also new NYC() in report and check steps. I agree that singletons are often a bad pattern, but in the case of a cache I believe it's the correct behavior (it should be a cache of source-maps that exist for the processes lifecycle).

}
}
return sourceMap
}

SourceMaps.prototype.remapCoverage = function (obj) {
var transformed = this.sourceMapCache.transformCoverage(
var transformed = this._sourceMapCache.transformCoverage(
libCoverage.createCoverageMap(obj)
)
return transformed.map.data
}

SourceMaps.prototype.reloadCachedSourceMaps = function (report) {
var _this = this
Copy link
Member

Choose a reason for hiding this comment

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

Since you are converting to an arrow function would you mind dropping _this and updating the whole function to use this directly?

@coreyfarrell coreyfarrell merged commit 18e04ba into master Apr 3, 2019
@coreyfarrell coreyfarrell mentioned this pull request Apr 3, 2019
@JaKXz JaKXz deleted the source-map-cache branch April 4, 2019 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants