feat(index): add support for a custom compare {Function} (options.compare) and generate {Function} (options.generate)
#41
Conversation
|
|
package.json
Outdated
| @@ -25,7 +25,8 @@ | |||
| "travis:lint": "npm run lint && npm run security", | |||
| "travis:test": "npm run test -- --runInBand", | |||
| "appveyor:test": "npm run test", | |||
| "defaults": "webpack-defaults" | |||
There was a problem hiding this comment.
Not entirely sure what happened with this... something weird must have happened at npm install. I'll fix.
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
=======================================
Coverage 17.47% 17.47%
=======================================
Files 2 2
Lines 103 103
Branches 13 13
=======================================
Hits 18 18
Misses 73 73
Partials 12 12
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
=======================================
Coverage 17.47% 17.47%
=======================================
Files 2 2
Lines 103 103
Branches 13 13
=======================================
Hits 18 18
Misses 73 73
Partials 12 12
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
===========================================
+ Coverage 17.47% 40.59% +23.11%
===========================================
Files 2 2
Lines 103 101 -2
Branches 13 12 -1
===========================================
+ Hits 18 41 +23
+ Misses 73 51 -22
+ Partials 12 9 -3
Continue to review full report at Codecov.
|
michael-ciniawsky
changed the title
{Function} (options.compare)
There was a problem hiding this comment.
@LukeSheard Do you have a working solution using the file contents (hash) instead of mtime? If so I would love to see it :)
| @@ -22,6 +22,7 @@ const defaults = { | |||
| cacheKey, | |||
| read, | |||
| write, | |||
| compare, | |||
There was a problem hiding this comment.
Do we may have a better name for this? (in)validate (but not sure)?
There was a problem hiding this comment.
Maybe, naming things is hard right? I like the idea of refactoring this so that it is validate and that you have to pas the validation status into the callback, i.e. true if the cache is still valid. Thoughts?
There was a problem hiding this comment.
I think the compare name is accurate. It's what "sort" uses, for example, as a name: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort
|
@michael-ciniawsky I don't have anything I can share unfortunately - we currently run a slightly forked version of cache-loader and do our business logic elsewhere. The hash comment was just an example taken from #34 . |
|
👍 I think the PR as is works perfectly fine. Is there a chance this can be merged in? |
|
@evilebottnawi Well, the functionallity didn't change at all, since it is defaulting to the previous comparing method, so the tests are there anyway, they didn't change. What test would you like to be added? Like adding a second test function? That is up to the user to provide one that works for them. |
|
One thing that I do think it needs is proper documentation in the README.md |
|
@evilebottnawi I'm super interested by this PR but I wonder what is the expected outcome here? I may be wrong, but if the idea is to offer a way to check file "fresheness" by comparing hashes, we probably need |
|
@jraoult Yeah, I agree - I just haven't gotten around to updating my PR with the parity with |
|
@evilebottnawi How do you propose testing this? The only tests I can see currently are for validation of the config options.. |
|
@LukeSheard we need add directory |
LukeSheard
changed the title
{Function} (options.compare){Function} (options.compare) and generate {Function} (options.generate)
|
@evilebottnawi I'm all for having a regression suite to test PRs agains. I've provided a working example in the It makes me feel quite like a scapegoat to be told I have to write a whole regression suite for a library which doesn't have any real tests right now - just to move a couple of functions around. :( |
|
Any reason why this can’t be merged now? |
| } | ||
|
|
||
| const compare = (data, callback) => { | ||
| return fs.readFile(data.depPath, (err, content) => { |
This add supports for a custom comparison function during the read phase, which allows the pitch phase to rely on something other than the stat of the module for comparison (e.g. the hash of the content of the file).