paulwalker / connect-static-expiry Goto Github PK

When using a host option without a protocol. I think this is related to nodejs/node#3361, or some other breaking change with url.format. I'm not sure what the proper behavior should be.

I get this error:

app.locals.furl = furl;
TypeError: Cannot set property 'furl' of undefined

app.locals is defined in express, not in connect.

So should this actually be express middleware rather than connect middleware? Or are there plans to make this usable from connect as well?

When I pass debug: false in the options arg, it get's lost in the call to setOptions. It loses it here in line 46:

this.options[opt] = opts[opt] || defaults[opt];

because opts['debug'] is false, it uses defaults['debug'] which is always true.

Because the dependency on findit is defined as wildcard, the recent change on find it breaks everything :-(

Pull request with actual version of dependency coming

Thanks for accepting my pull request #8!

I'm currently inclyding the change by linking directly to the git-repostory in my project, but would prefer to point to a released version instead.

If you want help I'm up for helping out with some of the maintaining of this module.

NPM audit reveals high vulnerability issues in dependency. This is fixable by revving fresh to 0.5.2

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High │ Regular Expression Denial of Service │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ fresh │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >= 0.5.2 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ static-expiry │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ static-expiry > fresh │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/526 │
└───────────────┴──────────────────────────────────────────────────────────────┘

I did the config as follows

app.use(expiry(app, {
    dir: path.join(__dirname, 'public'),
    duration: 31556900
}));
app.use(express.static(path.join(__dirname, 'public'), {
    maxage: config.oneDay
}, {
    etag: config.etagAllow
}, {
    dotfiles: config.Allowdotfiles
}));

and defined the route as <link rel="stylesheet" type="text/css" href="<%= furl(../vendor/main_Lib.min.css) %>">

but the resource fails to load?

Right now, the module's signature is expiry(app, furl). app is only used to attach the generated furl function to app.locals.furl.

This api is causing me some small issues, though, and seems like it could be improved a bit. Namely, with the current api, the caller has no way to know from the signature how app might be mutated, and no way to avoid the mutation (assigning furl to the local) if it's not desired. In my case, I don't need the app mutation at all (because I'm not using express' built-in templating, and just need to save the generated furl function for use elsewhere). Meanwhile, I have a function that conditionally returns different middleware to use, and it now must take app as an argument too, only to pass it down to expiry. That's making its ergonomics a bit clunky. I could generate a new/fake app in the function and pass that to expiry, but I can't do that with any certainty because I don't know that, (eg) in a future version, expiry won't actually need the real app.

So, I want to propose the following:

Expose the furl function as a property of the returned middleware (i.e., expiry(app, opts).furl) and then allow the app argument to be null; if its null, obviously you don't do app.locals.furl = furl assignment. This way, the function still always returns only the middleware, which preserves backwards compatibility while keeping a uniform interface (as opposed to, e.g., sometimes returning a [middleware, furl] pair) but it makes the furl function available to callers that don't want to mutate their app. And, by making app essentially optional, makes clear that app isn't actually required for the module to do its work.

Thoughts? If this sounds good, I'm happy to work up a branch.

If i run the app locally on my mac with NODE_ENV production the preCache functions as expected.

However in the deployment environment (Ubuntu dotcloud) I get the following error:

[www.0] Error: EISDIR, illegal operation on a directory
[www.0] at Object.fs.readSync (fs.js:381:19)
[www.0] at Object.fs.readFileSync (fs.js:208:28)
[www.0] at Object.md5 as fingerprint
[www.0] at fingerprintAssetUrl (/home/dotcloud/node_modules/static-expiry/index.js:157:27)
[www.0] at preCache (/home/dotcloud/node_modules/static-expiry/index.js:126:36)
[www.0] at expiry (/home/dotcloud/node_modules/static-expiry/index.js:282:47)
[www.0] at Function. (/home/dotcloud/rsync-1363713142908/server.js:27:13)
[www.0] at Function.app.configure (/home/dotcloud/node_modules/express/lib/application.js:396:61)
[www.0] at Object. (/home/dotcloud/rsync-1363713142908/server.js:21:5)
[www.0] at Module._compile (module.js:449:26)

By replacing the fingerprint method in the options I can see it is blowing up trying to open

./public/assets

Where assets is actually a directory, which is why readFileSync is throwing that error.

Any ideas why findIt is matching that folder as a file?

Hey Paul,

Can you publish the latest version to npm? I don't seem to have access to do that.

Cheers,
Ethan

Hi, Paul!

Can you update fresh dependency to the latest version (0.5.2 for now)?
In this version was fixed the vulnerability (https://snyk.io/vuln/npm:fresh:20170908).

And after that, can you publish to npm? The latest available version is still 0.0.11.

Cheers,
Alex.