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: add config values to hash salt #988

Merged
merged 4 commits into from
Feb 27, 2019

Conversation

taye
Copy link
Contributor

@taye taye commented Feb 10, 2019

This PR adds the instrumenter-related config options to the salt of the cache hash. This fixes a bug where code that has been cached without instrumentation or source maps is incorrectly reused after the options are later enabled, leading to missing instrumentation. This problem is more likely to come up in projects using babel-plugin-istanbul which recommends setting the sourceMap and instrument options to false.

Closes #552 and probably also #822 #822 #974 #980

Thank you to everyone maintaining and contributing to this project ❤️

@coveralls
Copy link

coveralls commented Feb 10, 2019

Coverage Status

Coverage increased (+0.2%) to 96.639% when pulling 2858ac9 on taye:fix-cache-invalidation into 8a5e222 on istanbuljs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.399% when pulling 987e4dd on taye:fix-cache-invalidation into 8a5e222 on istanbuljs:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.399% when pulling 987e4dd on taye:fix-cache-invalidation into 8a5e222 on istanbuljs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.399% when pulling 987e4dd on taye:fix-cache-invalidation into 8a5e222 on istanbuljs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.399% when pulling 987e4dd on taye:fix-cache-invalidation into 8a5e222 on istanbuljs:master.

Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

Please hold off on any changes related to potentially disabling cache based on instrument or instrumenter for now.

lib/hash.js Outdated Show resolved Hide resolved
lib/hash.js Show resolved Hide resolved
lib/hash.js Show resolved Hide resolved
@coreyfarrell coreyfarrell requested review from JaKXz and bcoe February 10, 2019 17:15
@JaKXz JaKXz requested a review from coreyfarrell February 25, 2019 00:52
Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for contributing!

Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

@bcoe @coreyfarrell it would be cool if all the issues @taye mentioned could be closed in the merge commit but I'll leave that judgement call to you

@coreyfarrell
Copy link
Member

I think #522 is the only one this should close. The other tickets are either unrelated or of undetermined cause.

@coreyfarrell coreyfarrell merged commit 7ac325d into istanbuljs:master Feb 27, 2019
@coreyfarrell coreyfarrell mentioned this pull request Apr 3, 2019
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.

Cache key doesn't take --source-map into account
4 participants