Fix #146. Cache is valid while fetching assets.

[zeroby0]

Fixes #146.

While calculating hash, assetCache.isCacheValid() returns false while the image is being fetched, and that returns in a different hash digest.

When 11ty is run the second time, assetCache.isCacheValid() returns true which generates a different hash in the filename, resulting in 404s.

This PR fixes the race condition by marking cache valid while the image is being fetched. More concretely, while assetCache.needsToFetch() returns true.

Don't merge

Please see discussion below.

[vntw]

Hey, thanks for the fix! I had to debug this as well and ended up at this line - can confirm that the hash changing on the second run is fixed by this PR.

Would be great to have a fix released so users can remove the fork/don't have to build twice 🙂

[zeroby0]

@vntw

FWIW, I don't think the bug is actually fixed. I checked the source for assetCache, and isCacheValid uses needsToFetch
internally.

https://github.com/11ty/eleventy-fetch/blob/28201ee82f023f9a45f375da22bdf7aecd37feea/src/AssetCache.js#L173-L175

isCacheValid(duration) {
    return this.needsToFetch(duration || this.defaultDuration) === false;
}

So what the patch effectively does is:

if (needsToFetch === true) {
    hash.update(`ValidCache:true`);
} else {  // needsToFetch is false
    hash.update(`ValidCache: needsToFetch === false)`); // always evaluates to true now.   
}

This effectively disables using cache validity in hash. So you might as well set options.useCacheValidityInHash to false and have the same effect.

This bug probably can't be solved without co-operation from the cache. The primary reason for this bug is hashing the url of images instead of the image content (which is done for local files). Which we probably can't avoid because statsBySync needs to provide the hash immediately, synchronously. If eleventy-fetch can maintain a counter that updates every time the cached image expires, or when the image changes, we can mitigate this bug to some extent.

This was a concern I had here: #116 (review).

I'm slightly embarassed that I didn't realise this when submitting the PR. So this is why caching and naming things is the hardest thing in CS.

I'll try to think of a work around, but until then, triggering builds twice, both in dev and in deployment, might be the best alternative. Unless you're sure that your remote images wont change, in that case you can set options.useCacheValidityInHash to false and forget about this issue.