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

Conversation

coreyfarrell
Copy link
Member

@bcoe this (combined with istanbuljs/istanbuljs#375) seems to fix #1049 (and likely others like it).

@bcoe
Copy link
Member

bcoe commented Apr 21, 2019

@istanbuljs/core awesome work 👍 weirdly, npm link isn't working well for me to test my changes in istanbul-lib-source-map. might need to release a next release, just for testing purposes.

lib/source-maps.js Outdated Show resolved Hide resolved
function SourceMaps (opts) {
this.cache = opts.cache
this.cacheDirectory = opts.cacheDirectory
this.loadedMaps = {}
this._sourceMapCache = sourceMapCache
}

SourceMaps.prototype.purgeCache = function () {
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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

This is unneeded since we always limit ourselves to a single NYC
instance.
@coreyfarrell coreyfarrell marked this pull request as ready for review April 21, 2019 22:17
@coreyfarrell
Copy link
Member Author

might need to release a next release, just for testing purposes

For istanbul-lib-source-maps I think definitely. Once we do that I think we should ping Simen from jest to ensure it doesn't cause regressions for them.

index.js Show resolved Hide resolved
@coreyfarrell coreyfarrell merged commit 3d9eaa4 into istanbuljs:master Apr 24, 2019
@coreyfarrell coreyfarrell deleted the sm-purge-cache branch May 2, 2019 14:07
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.

Typescript coverage doesn't include some lines after 13.2.0 update
3 participants