expressjs / serve-static

Serve static files
MIT License
1.39k stars 228 forks source link

Added support for serving static gzipped files. #63

Closed TheUltDev closed 8 years ago

TheUltDev commented 8 years ago

Hi @dougwilson, I appreciate your feedback with the last PR, I've corrected most of the items you listed. There were two things that fell short in this PR. First was true directory detection (I added a simple check for an extension as I don't need to serve gzipped files without an extension). The other thing was refreshing the gzip cache, I didn't add that in as it'd either be a lookup for each request or some sort of watcher. I don't have much interest in adding those in as it's a lot of complexity for an unwanted feature (speaking on my behalf).

Feel free to close this out, I figured I'd give it a second chance though, I will say I don't consider https://github.com/code42day/connect-gzip-static an option as it's outdated, a lot of my initial PR was based on that repository, including issues you pointed out. Also, it's more or less a worse fork of this project so you lose a lot of nice fixes that this project provides.

Fixed multiple compatibility issues:

steve-gray commented 8 years ago

Where I've been working, we've found that we don't specifically need to handle this inside the static middleware. Have you checked out the npm package compression? Simply include it in the connect/express pipeline prior to the as-is serve-static module and you'll get this capability.

As such, does this need to be baked into the middleware here @Cavitt? I doubt this module is getting much love/change lately - but it does what it needs to.

dougwilson commented 8 years ago

I doubt this module is getting much love/change lately

What makes you say this? Is there something you expect me to be doing here that is not happening? What can I do that would signal that this module is still loved?

I'm sorry I forgot to reply to this PR when it first popped up (but as you can see from the message, I was actively involved in the first version of this PR, ultimately saying this is not a desired feature of this module). There is currently discussion of adding this feature into the "send" module, which is the core logic of this module and the correct place to implement this feature, if it were to be implemented.

@steve-gray, I try to respond to all messages in a timely manor, but this is free & open source software, where I am not paid to do this.

steve-gray commented 8 years ago

Hi @dougwilson,

My wording was poor - apologies - it wasn't meant as a dig - but rather a reflection that this module is pretty much done and living up to it's clear and defined purpose, and it's unlikely that there's benefit to start turning this piece into a kitchen-sink type middleware, which a lot of projects seem to succumb to. serve-static is utterly my go-to middleware for this kind content serving (combined usually with connect-mount or similiar).

dougwilson commented 8 years ago

Ah, gotcha, @steve-gray! Yes, this is pretty much "done" and I have learned to strive to make the modules not become kitchen sinks, as it leads to just more bugs and becomes harder & harder to maintain. Typically there is room to allow others to extend the module, and this may be one of those cases, where people may want to replace some parts (though the source is not that large and being MIT can easily be modified in private).

I do try to put love into them and keep up with the changing ecosystem, at least in that the "done"ness of the modules keeps functioning with each new release of Node.js. It's very similar to the connect module itself :)

Sorry I guess I misinterpreted the comment, but this comment is pretty much aligned with how I feel about these modules.