Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export fingerprint #21

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

International
Copy link

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
Copy link
Owner

bminer commented Jan 18, 2016

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
Copy link
Author

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
Copy link
Owner

bminer commented Jan 18, 2016

@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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants