-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Bump dependencies, refactor hash() option #2
Bump dependencies, refactor hash() option #2
Conversation
var data = [ownHash || getOwnHash(), input]; | ||
if (salt) { | ||
data.push(salt); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newlines before and after if blocks.
I forget to follow this myself half the time. But @sindresorhus is going to introduce it into XO as soon as the rule exists, so might as well start following the rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean:
var data = [ownHash || getOwnHash(), input];
if (salt) {
data.push(salt);
}
if (hashData) {
data = data.concat(hashData(input, metadata));
}
Or:
var data = [ownHash || getOwnHash(), input];
if (salt) {
data.push(salt);
}
if (hashData) {
data = data.concat(hashData(input, metadata));
}
Cause there's more if-blocks to change in case of the latter. Which makes me thing this should wait until XO complains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter.
We can do a cleanup commit later. That's fine.
106f900
to
c74af0f
Compare
@jamestalmage force-pushed to bump XO to |
|
||
Provide additional data that should be included in the hash. By default `metadata` is not taken into account. | ||
|
||
#### onHash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more #
A few comments on the docs. Otherwise, LGTM |
Both AVA and nyc use the `hash` option to ensure some extra data is included in the hash, and to store a mapping from the hash to a file. They also need to (correctly!) use md5-hex to generate the hash. This commit replaces the `hash` option by a `hashData` function an an `onHash` callback. `hashData` receives the original input and additional data and may return a string, buffer or array of either. This is then included in the hash. `onHash` is called with the input, additional data, and the resulting hash. Calling code can use this to maintain mappings. This is a breaking change.
Include the package-hash result for caching-transform itself in the resulting hash. This will ensure the hash changes if caching-transform is updated, e.g. if it changes its storage format.
c74af0f
to
45d43dc
Compare
Force-pushed again. Updated readme (and fixed an existing typo). Bumped |
Apologies for combining all of this into one PR :) First couple commits update some dev dependencies, add linting and clean up some whitespace. I've also removed unused spies from the tests.
The real meat of this PR is replacing the
hash
option. Both AVA and nyc use this option to ensure some extra data is included in the hash, and to store a mapping from the hash to a file.Overriding the hash function in order to achieve these goals seems too error-prone, especially since it relies on the consuming code to properly invoke
md5-hex
. This PR replaces thehash
option by ahashData
function an anonHash
callback:hashData
receives the original input and additional data and may return a string, buffer or array of either. This is then included in the hash.onHash
is called with the input, additional data, and the resulting hash. Calling code can use this to maintain mappings.I've tried this locally with AVA and it works fine. Plus AVA no longer needs a dependency on
md5-hex
which is nice.This is a breaking change though!
With the above changes it becomes possible to include a package hash for
caching-precompiler
itself, which should be useful if the storage format is changed. I believe that'll be necessary for istanbuljs/nyc#217, which requires multiple files to be generated for a single input.