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

Bump dependencies, refactor hash() option #2

Merged
merged 8 commits into from May 16, 2016

Conversation

novemberborn
Copy link
Contributor

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 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.

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.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 106f900 on novemberborn:update-and-hash-refactor into 508395e on jamestalmage:master.

var data = [ownHash || getOwnHash(), input];
if (salt) {
data.push(salt);
}
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@novemberborn
Copy link
Contributor Author

@jamestalmage force-pushed to bump XO to 0.15.0. Let me know what you want me to do with the if conditions. Have your concerns about package-hash been alleviated?

@coveralls
Copy link

coveralls commented May 9, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling c74af0f on novemberborn:update-and-hash-refactor into 508395e on jamestalmage:master.


Provide additional data that should be included in the hash. By default `metadata` is not taken into account.

#### onHash
Copy link
Member

Choose a reason for hiding this comment

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

one more #

@jamestalmage
Copy link
Member

A few comments on the docs. if statements are fine as is until we find a better linter rule.

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.
@novemberborn
Copy link
Contributor Author

Force-pushed again. Updated readme (and fixed an existing typo). Bumped package-hash to the freshly published 1.2.0.

@coveralls
Copy link

coveralls commented May 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 45d43dc on novemberborn:update-and-hash-refactor into 508395e on jamestalmage:master.

@jamestalmage jamestalmage merged commit 8195916 into istanbuljs:master May 16, 2016
@novemberborn novemberborn deleted the update-and-hash-refactor branch May 16, 2016 17:21
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.

None yet

3 participants