Clean up Utils#150
Conversation
| bucket: "static", | ||
| plugin: "faucet-pipeline-static" | ||
| }, | ||
| assets: { |
There was a problem hiding this comment.
I think this file is also the place where we would mark the old name as deprecated.
There was a problem hiding this comment.
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...
FND
left a comment
FND
left a comment
There was a problem hiding this comment.
Seems reasonable to me, though I've kinda run out of brainspace for the moment; let's conclude faucet-pipeline/faucet-pipeline-static#71 first.
| } | ||
|
|
||
| // exported for testing | ||
| exports.generateFingerprint = generateFingerprint; |
There was a problem hiding this comment.
| exports.generateFingerprint = generateFingerprint; | |
| exports._generateFingerprint = generateFingerprint; |
That makes it explicit; cf. _determinePlugins.
|
|
||
| function generateFingerprint(filepath, data) { | ||
| let filename = path.basename(filepath); | ||
| let ext = filename.indexOf(".") === -1 ? "" : "." + filename.split(".").pop(); |
There was a problem hiding this comment.
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.)
| exports.generateFingerprint = generateFingerprint; | ||
|
|
||
| function generateHash(str) { | ||
| let hash = crypto.createHash("md5"); |
There was a problem hiding this comment.
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.
| let fs = require("fs"); | ||
| let path = require("path"); |
There was a problem hiding this comment.
Might as well do this while we're at it:
| 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; |
There was a problem hiding this comment.
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. 🤷
| bucket: "static", | ||
| plugin: "faucet-pipeline-static" | ||
| }, | ||
| assets: { |
There was a problem hiding this comment.
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...
|
Thanks for the feedback! I plan to incorporate that this week. I worked on two things we discussed in a chat:
|
This is a breaking change. * FileFinder is removed (pending based on the static/images work) * resolvePath is moved into util/index.js (this is a breaking change for aiur) * generateFingerprint is moved into lib/manager.js as it is the only place where it is used (the test is moved accordingly) * SerializedRunner is moved to lib, as it is only used in core
3bf9b98 to
1174e8a
Compare
|
I rebased this branch on main-revised, and pulled out the parts into separate PRs to make it easier to discuss. This is the "all the other stuff" PR. I don't think it makes sense in its current form - it is more a reminder for things to look at a bit later. |
2 participants
Clean up utils
Looking into static, I noticed that we use the
utilsless than I thought.This is a breaking change.