Fix bug with not properly de-duping fetches with custom fetching function sources #32 Fixes #52

Author: zachleat

eleventy-fetch.js

@@ -67,21 +67,28 @@ queue.on("active", () => {
 
 let inProgress = {};
 
-function queueSave(source, queueCallback) {
-	if (!inProgress[source]) {
-		inProgress[source] = queue.add(queueCallback).finally(() => {
-			delete inProgress[source];
+function queueSave(source, queueCallback, options) {
+	let sourceKey;
+	if(typeof source === "string") {
+		sourceKey = source;
+	} else {
+		sourceKey = RemoteAssetCache.getUid(source, options);
+	}
+
+	if (!inProgress[sourceKey]) {
+		inProgress[sourceKey] = queue.add(queueCallback).finally(() => {
+			delete inProgress[sourceKey];
 		});
 	}
 
-	return inProgress[source];
+	return inProgress[sourceKey];
 }
 
 module.exports = function (source, options) {
 	let mergedOptions = Object.assign({}, globalOptions, options);
 	return queueSave(source, () => {
 		return save(source, mergedOptions);
-	});
+	}, mergedOptions);
 };
 
 Object.defineProperty(module.exports, "concurrency", {

src/AssetCache.js

@@ -9,22 +9,32 @@ const debug = require("debug")("Eleventy:Fetch");
 class AssetCache {
 	#customFilename;
 
-	constructor(url, cacheDirectory, options = {}) {
-		let uniqueKey;
+	static getCacheKey(source, options) {
+		// RemoteAssetCache sends this an Array, which skips this altogether
 		if (
-			(typeof url === "object" && typeof url.then === "function") ||
-			(typeof url === "function" && url.constructor.name === "AsyncFunction")
+			(typeof source === "object" && typeof source.then === "function") ||
+			(typeof source === "function" && source.constructor.name === "AsyncFunction")
 		) {
 			if(typeof options.formatUrlForDisplay !== "function") {
-				throw new Error("When caching an arbitrary promise source, options.formatUrlForDisplay is required.");
+				throw new Error("When caching an arbitrary promise source, an options.formatUrlForDisplay() callback is required.");
 			}
 
-			uniqueKey = options.formatUrlForDisplay();
-		} else {
-			uniqueKey = url;
+			return options.formatUrlForDisplay();
 		}
 
+		return source;
+	}
+
+	constructor(url, cacheDirectory, options = {}) {
+		let uniqueKey;
+		// RemoteAssetCache passes in an array
+		if(Array.isArray(uniqueKey)) {
+			uniqueKey = uniqueKey.join(",");
+		} else {
+			uniqueKey = AssetCache.getCacheKey(url, options);
+		}
 		this.uniqueKey = uniqueKey;
+
 		this.hash = AssetCache.getHash(uniqueKey, options.hashLength);
 		this.cacheDirectory = cacheDirectory || ".cache";
 		this.defaultDuration = "1d";

src/RemoteAssetCache.js

@@ -1,24 +1,34 @@
-const fs = require("fs");
-const fsp = fs.promises; // Node 10+
-
 const AssetCache = require("./AssetCache");
-const debug = require("debug")("Eleventy:Fetch");
+// const debug = require("debug")("Eleventy:Fetch");
 
 class RemoteAssetCache extends AssetCache {
 	constructor(url, cacheDirectory, options = {}) {
 		let cleanUrl = url;
 		if (options.removeUrlQueryParams) {
-			if (typeof cleanUrl !== "string") {
-				throw new Error(
-					"The `removeUrlQueryParams` option requires the cache source to be a string. Received: " +
-						typeof url,
-				);
-			}
-
 			cleanUrl = RemoteAssetCache.cleanUrl(cleanUrl);
 		}
 
-		let cacheKey = [cleanUrl];
+		// Must run after removeUrlQueryParams
+		let displayUrl = RemoteAssetCache.convertUrlToString(cleanUrl, options);
+		let cacheKeyArray = RemoteAssetCache.getCacheKey(displayUrl, options);
+
+		super(cacheKeyArray, cacheDirectory, options);
+
+		this.url = url;
+		this.options = options;
+
+		this.displayUrl = displayUrl;
+	}
+
+	static getUid(source, options) {
+		let displayUrl = RemoteAssetCache.convertUrlToString(source, options);
+		let cacheKeyArray = RemoteAssetCache.getCacheKey(displayUrl, options);
+		return cacheKeyArray.join(",");
+	}
+
+	static getCacheKey(source, options) {
+		// Promise sources are handled upstream
+		let cacheKey = [source];
 
 		if (options.fetchOptions) {
 			if (options.fetchOptions.method && options.fetchOptions.method !== "GET") {
@@ -29,29 +39,33 @@ class RemoteAssetCache extends AssetCache {
 			}
 		}
 
-		super(cacheKey, cacheDirectory, options);
-
-		this.url = url;
-		this.options = options;
-
-		// Important: runs after removeUrlQueryParams
-		this.displayUrl = this.formatUrlForDisplay(cleanUrl);
+		return cacheKey;
 	}
 
 	static cleanUrl(url) {
-		let cleanUrl = new URL(url);
+		if(typeof url !== "string" && !(url instanceof URL)) {
+			return url;
+		}
+
+		let cleanUrl;
+		if(typeof url === "string") {
+			cleanUrl = new URL(url);
+		} else if(url instanceof URL) {
+			cleanUrl = url;
+		}
+
 		cleanUrl.search = new URLSearchParams([]);
+
 		return cleanUrl.toString();
 	}
 
-	formatUrlForDisplay(url) {
-		if (
-			this.options.formatUrlForDisplay &&
-			typeof this.options.formatUrlForDisplay === "function"
-		) {
-			return this.options.formatUrlForDisplay(url);
+	static convertUrlToString(source, options = {}) {
+		let { formatUrlForDisplay } = options;
+		if (formatUrlForDisplay && typeof formatUrlForDisplay === "function") {
+			return formatUrlForDisplay(source);
 		}
-		return url;
+
+		return source;
 	}
 
 	get url() {

test/QueueTest.js

@@ -3,8 +3,6 @@ const fs = require("fs");
 const Cache = require("../");
 const RemoteAssetCache = require("../src/RemoteAssetCache");
 
-let fsp = fs.promises;
-
 test("Double Fetch", async (t) => {
 	let pngUrl = "https://www.zachleat.com/img/avatar-2017-big.png";
 	let ac1 = Cache(pngUrl);
@@ -40,3 +38,34 @@ test("Double Fetch (dry run)", async (t) => {
 	// file is now accessible
 	t.false(forTestOnly.hasCacheFiles());
 });
+
+test("Double Fetch async function (dry run)", async (t) => {
+	let expected = { mockKey: "mockValue" };
+
+	async function fetch() {
+		return Promise.resolve(expected);
+	};
+
+let ac1 = Cache(fetch, {
+	dryRun: true,
+	formatUrlForDisplay() {
+		return "fetch-1";
+	},
+});
+let ac2 = Cache(fetch, {
+	dryRun: true,
+	formatUrlForDisplay() {
+		return "fetch-2";
+	},
+});
+
+	// Make sure we only fetch once!
+	t.not(ac1, ac2);
+
+	let result1 = await ac1;
+	let result2 = await ac2;
+
+	t.deepEqual(result1, result2);
+	t.deepEqual(result1, expected);
+	t.deepEqual(result2, expected);
+});