Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
let server = require("./server");
let { pluginsByBucket } = require("./plugins");
let { AssetManager } = require("./manager");
let { resolvePath } = require("./util/resolve");
let { abort, repr } = require("./util");
let { SerializedRunner } = require("./util/runner");
let { abort, repr, resolvePath } = require("./util");
let { SerializedRunner } = require("./runner");
let browserslist = require("browserslist");

exports.faucetDispatch = async function faucetDispatch(referenceDir, config,
Expand Down
21 changes: 19 additions & 2 deletions lib/manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

let { Manifest } = require("./manifest");
let { createFile } = require("./util/files");
let { resolvePath } = require("./util/resolve");
let { reportFileStatus, abort, generateFingerprint } = require("./util");
let { reportFileStatus, abort, resolvePath } = require("./util");
let path = require("path");
let crypto = require("crypto");

exports.AssetManager = class AssetManager {
constructor(referenceDir, { manifestConfig, fingerprint, exitOnError } = {}) {
Expand Down Expand Up @@ -58,3 +58,20 @@ exports.AssetManager = class AssetManager {
return this.manifest.set(originalPath, actualPath, targetDir);
}
};

function generateFingerprint(filepath, data) {
let filename = path.basename(filepath);
let ext = filename.indexOf(".") === -1 ? "" : "." + filename.split(".").pop();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we not using path.extname here? (See potentially related discussion for faucet-static/static-images.)

(Though we might reasonably opt not to risk changing the implementation here, due to backwards-compatibility concerns.)

let name = ext.length === 0 ? filename : path.basename(filepath, ext);
let hash = generateHash(data);
return path.join(path.dirname(filepath), `${name}-${hash}${ext}`);
}

// exported for testing
exports.generateFingerprint = generateFingerprint;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
exports.generateFingerprint = generateFingerprint;
exports._generateFingerprint = generateFingerprint;

That makes it explicit; cf. _determinePlugins.


function generateHash(str) {
let hash = crypto.createHash("md5");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we perhaps change this to SHA-256? I realize that might be a backwards-compatibility concern though...

My results suggest that you should probably not be using MD5. MD5 is slower than SHA-256 and not as safe.

JavaScript hashing speed comparison: MD5 versus SHA-256

hash.update(str);
return hash.digest("hex");
}
3 changes: 1 addition & 2 deletions lib/manifest.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
"use strict";

let { createFile } = require("./util/files");
let { resolvePath } = require("./util/resolve");
let { abort } = require("./util");
let { abort, resolvePath } = require("./util");
let path = require("path");

exports.Manifest = class Manifest {
Expand Down
4 changes: 4 additions & 0 deletions lib/plugins.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ let DEFAULTS = [{
key: "static",
bucket: "static",
plugin: "faucet-pipeline-static"
}, {
key: "assets",
bucket: "static",
plugin: "faucet-pipeline-static"
}, {
key: "images",
bucket: "static",
Expand Down
File renamed without changes.
File renamed without changes.
59 changes: 0 additions & 59 deletions lib/util/files/finder.js

This file was deleted.

42 changes: 25 additions & 17 deletions lib/util/index.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
"use strict";

let fs = require("fs");
let path = require("path");
Comment on lines +3 to 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well do this while we're at it:

Suggested change
let fs = require("fs");
let path = require("path");
let fs = require("node:fs");
let path = require("node:path");

let crypto = require("crypto");

exports.abort = abort;
exports.repr = repr;
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I had intentionally moved exports to the top (yay hoisting) so you don't have to go searching for them in the middle of random implementation details. 🤷


// reports success or failure for a given file path (typically regarding
// compilation or write operations)
Expand All @@ -26,28 +23,39 @@ exports.loadExtension = async (pkg, errorMessage, supplier = pkg) => {
}
};

exports.generateFingerprint = (filepath, data) => {
let filename = path.basename(filepath);
let ext = filename.indexOf(".") === -1 ? "" : "." + filename.split(".").pop();
let name = ext.length === 0 ? filename : path.basename(filepath, ext);
let hash = generateHash(data);
return path.join(path.dirname(filepath), `${name}-${hash}${ext}`);
};

function abort(msg, code = 1) {
console.error(msg);
process.exit(code);
}
exports.abort = abort;

function repr(value, jsonify = true) {
if(jsonify) {
value = JSON.stringify(value);
}
return `\`${value}\``;
}
exports.repr = repr;

function generateHash(str) {
let hash = crypto.createHash("md5");
hash.update(str);
return hash.digest("hex");
}
exports.resolvePath = (filepath, referenceDir, { enforceRelative } = {}) => {
if(/^\.?\.\//.test(filepath)) { // starts with `./` or `../`
return path.resolve(referenceDir, filepath);
} else if(enforceRelative) {
abort(`ERROR: path must be relative: ${repr(filepath)}`);
} else { // attempt via Node resolution algorithm
try {
return require.resolve(filepath, { paths: [referenceDir] });
} catch(err) {
// attempt to resolve non-JavaScript package references by relying
// on typical package paths (simplistic approximation of Node's
// resolution algorithm)
let resolved = path.resolve(referenceDir, "node_modules", filepath);
try {
fs.statSync(resolved); // ensures file/directory exists
} catch(_err) {
abort(`ERROR: could not resolve ${repr(filepath)}`);
}
return resolved;
}
}
};
29 changes: 0 additions & 29 deletions lib/util/resolve.js

This file was deleted.

17 changes: 16 additions & 1 deletion test/test_manager.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use strict";

let { AssetManager } = require("../lib/manager");
let { AssetManager, generateFingerprint } = require("../lib/manager");
let { describe, it, before, after } = require("node:test");
let path = require("path");
let assert = require("assert");
Expand Down Expand Up @@ -51,3 +51,18 @@ describe("asset manager", () => {
}, /exit 1/);
});
});

describe("fingerprinting", () => {
it("generates a content-dependent hash", () => {
let fingerprint = generateFingerprint("/path/to/foo.js", "lorem ipsum");
assertSame(fingerprint, "/path/to/foo-80a751fde577028640c419000e33eba6.js");

fingerprint = generateFingerprint("/path/to/bar.js", "dolor sit amet");
assertSame(fingerprint, "/path/to/bar-7afed6210e0b8fce023f06abd4490fa0.js");
});

it("supports files without extension", () => {
let fingerprint = generateFingerprint("/path/to/baz", "lipsum");
assertSame(fingerprint, "/path/to/baz-8047cfaac755e5c7f77af066123980a5");
});
});
4 changes: 4 additions & 0 deletions test/test_plugins.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ let DEFAULTS = {
bucket: "static",
plugin: "faucet-pipeline-static"
},
assets: {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this file is also the place where we would mark the old name as deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not currently sure how exactly we'd do that, especially because plugins can override these defaults? I guess we'll have to put on our hazmat suits and wade into the implementation eventually...

bucket: "static",
plugin: "faucet-pipeline-static"
},
images: {
bucket: "static",
plugin: "faucet-pipeline-images"
Expand Down
2 changes: 1 addition & 1 deletion test/test_runner.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use strict";

let { SerializedRunner } = require("../lib/util/runner");
let { SerializedRunner } = require("../lib/runner");
let { describe, it } = require("node:test");
let { strictEqual: assertSame, deepStrictEqual: assertDeep } = require("assert");

Expand Down
Loading