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: Purge source-map cache before reporting if cache is disabled. #1080

Merged
merged 2 commits into from Apr 24, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -67,6 +67,7 @@ if ([

nyc.writeProcessIndex()

nyc.maybePurgeSourceMapCache()
if (argv.checkCoverage) {
nyc.checkCoverage({
lines: argv.lines,
@@ -236,6 +236,12 @@ class NYC {
return this._transform(code, filename)
}

maybePurgeSourceMapCache () {
if (!this.cache) {
This conversation was marked as resolved by JaKXz

This comment has been minimized.

Copy link
@JaKXz

JaKXz Apr 22, 2019

Member

Maybe I'm crazy but isn't this clause written backwards? My guess is that it should be !!this.cache but I don't know for sure, just wanted to ask to be sure.

This comment has been minimized.

Copy link
@coreyfarrell

coreyfarrell Apr 22, 2019

Author Member

The code is correct. lib/source-map.js needs to cache source-maps in memory during execution of nyc. But if caching is disabled then we need to purge the caches before running reports.

This comment has been minimized.

Copy link
@JaKXz

JaKXz Apr 22, 2019

Member

Ah. The naming got me.

this.sourceMaps.purgeCache()
}
}

_transformFactory (cacheDir) {
const instrumenter = this.instrumenter()
let instrumented
@@ -4,14 +4,20 @@ const libSourceMaps = require('istanbul-lib-source-maps')
const fs = require('fs')
const path = require('path')

const sourceMapCache = libSourceMaps.createSourceMapStore()
let sourceMapCache = libSourceMaps.createSourceMapStore()
This conversation was marked as resolved by coreyfarrell

This comment has been minimized.

Copy link
@bcoe

bcoe Apr 21, 2019

Member

with your refactor to stop making new instances of Nyc, perhaps you're right that this doesn't need to be a singleton?

function SourceMaps (opts) {
this.cache = opts.cache
this.cacheDirectory = opts.cacheDirectory
this.loadedMaps = {}
this._sourceMapCache = sourceMapCache
}

SourceMaps.prototype.purgeCache = function () {
sourceMapCache = libSourceMaps.createSourceMapStore()

This comment has been minimized.

Copy link
@bcoe

bcoe Apr 21, 2019

Member

I don't understand why we'd need to throw out source maps like this, might be worth digging into things a bit more, and figuring out how we end up with incompatible source-maps in the first place.

I'm not opposed to landing this as a patch as a temporary measure, but it feels like we're continuing to complicate our source-map and caching logic, which is already a mess. I think you're right that we just need to get way more test cases in the codebase.

This comment has been minimized.

Copy link
@coreyfarrell

coreyfarrell Apr 21, 2019

Author Member

Same reason we need to throw out transform results (they set cache: false).

this._sourceMapCache = sourceMapCache
this.loadedMaps = {}
}

SourceMaps.prototype.extractAndRegister = function (code, filename, hash) {
var sourceMap = convertSourceMap.fromSource(code) || convertSourceMap.fromMapFileSource(code, path.dirname(filename))
if (sourceMap) {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.