bminer / node-static-asset

Static asset manager for Node.JS and Express
MIT License
48 stars 13 forks source link

Export fingerprint #21

Open International opened 8 years ago

International commented 8 years ago

Hi Blake, I needed the ability to fingerprint assets from more than one directory, and thus use the middleware multiple times in my code. However, because the middleware was exporting assetFingerprint, only assets from one of the folders would be fingerprinted. Thus, I forked the repo, to allow exporting the function under different names. Could you please have a look over the pull request, and if you consider it useful, perhaps consider merging it? :) As a disclaimer, I'm not a node.js developer, so it might be that my code is not entirely up to community standards. Any feedback would be appreciated.

Thanks for your great work!

bminer commented 8 years ago

Thank you for the PR!

I've given this some thought... I agree that one should have the ability to register fingerprints for multiple root paths, but I don't really like the idea of exposing req.assetFingerprint, req.secondaryAssetFingerprint, etc. Maybe one could access a particular req.assetFingerprint function for a particular rootPath like this:

// Setup middleware
var staticAsset = require('static-asset');
app.use(staticAsset(__dirname + "/public/") );
app.use(staticAsset(__dirname + "/public2/") );

// ... then later...

// Normal usage (for __dirname + "/public/" root path)
res.type("text/plain").send("The URL fingerprint for jQuery is: " +
        req.assetFingerprint("/js/jquery.min.js") );
// This next line is equivalent to above
res.type("text/plain").send("The URL fingerprint for jQuery is: " +
        req.assetFingerprint[0]("/js/jquery.min.js") );
// This next line is for the second middleware's `rootPath`
res.type("text/plain").send("The URL fingerprint for jQuery is: " +
        req.assetFingerprint[1]("/js/jquery.min.js") );
// Optionally, one can use the `rootPath` instead of numeric index
res.type("text/plain").send("The URL fingerprint for jQuery is: " +
        req.assetFingerprint[__dirname + "/public2/"]("/js/jquery.min.js") );

Thoughts on this approach? The same usage would apply for view helper functions.

International commented 8 years ago

I think i like the last approach the best, as using indexes might be too error-prone. Or what do you think if we'd give an optional alias when using the middleware, something like:

app.use(staticAsset(__dirname + "/public/", {fingerprintAlias: "public1") );
app.use(staticAsset(__dirname + "/public2/", {fingerprintAlias: "public2") );
....
res.type("text/plain").send("The URL fingerprint for jQuery is: " +
        req.assetFingerprint["public2"]("/js/jquery.min.js") );

The reason I suggest an alias is that it might lead to the fingerprint not being matched if we always use the entire path, What do you think?

bminer commented 8 years ago

@International - Yeah, I like that approach the best, as well. The staticAsset function already takes two arguments, but the second strategy parameter can be changed to options Object with strategy and fingerprintAlias keys. This would technically be a breaking change, but it's about time for a v1 release of this lib anyway.

staticAsset(__dirname + "/public/", {"strategy": ..., "fingerprintAlias": "public1"})

I still like the ability to reference the assetFingerprint function using the full root path and the fingerprintAlias, if specified. As for numeric indices... I'll agree with you that they might be too error prone and therefore should not be a part of the implmentation. This will be a bit nicer and pollute the req namespace a bit less.

You're welcome to take a shot at the implementation on this PR (or another if you'd like). Otherwise, I will implement something when I find some free time next month. Thanks again!

International commented 8 years ago

cool, thanks @bminer , i'll try to get something done soon :)