feat: add support for serving statically compressed files#158
Conversation
mcollina
left a comment
There was a problem hiding this comment.
Good work! Could you add a test with wildard: false? thanks
How thorough should this check be? Trying it out locally I can duplicate the tests I wrote already and they pass with that option, but not sure if that is the right way to author the tests. How can I write a test that verifies that option works for the cases I have set up without all the duplication? |
|
That'd be enough! |
|
Resolved everything so far. Should we get more eyes on this? |
|
Awesome! Thank you for the patience with the review @mcollina. What are the next steps? |
delvedor
left a comment
There was a problem hiding this comment.
I think it would be useful to support a separate path option for the compressed files. In this way, it might be easier to write automation and remove old files when generating new ones. What do you think?
|
@delvedor I think it could be useful, but I am not certain it would be worth the complexity. const options: FastifyStaticOptions = {
// ...
preCompressed?: true | PathLike
}I tried my best to get the same behavior from |
|
I started trying out this branch in a side project and I'm finding that with the new behavior I also have to implement |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Eomm
left a comment
There was a problem hiding this comment.
Adding some consideration about the header evaluation.
I would add the separate compressed path feature in a follow-up pr
|
Could you resolve the conflics? I think it is necessary to address this use case for a safe feature |
…aling with weighted encodings.
|
Hey folks. I updated this PR with the latest version and added I didn't add any tests with the weighted encoding format since I think all that handled by Thank you all for your patience! |
|
|
||
| #### `preCompressed` | ||
|
|
||
| Default: `false` |
There was a problem hiding this comment.
I'm curious why this defaults to false. Is the expectation that people are typically serving precompressed assets with something like nginx in front of Fastify?
As for the general behaviour, what I've observed is that people typically dump all of their assets into a build dir of some sort (as you've shown with public/ below) and would expect those to be served by whatever is handling their static assets. I guess I'm concerned that this default is unintuitive, and will generate a bunch of "why doesn't this work" issues for the fastify team.
You may choose to skip compression for smaller files that don't benefit from it.
Usually build processes just avoid compressing these in the first place. I don't think the application should be making this decision, personally. All the webpack compression plugins for example, take a minimum size to compress & save.
This is all very anecdotal, so please feel free to correct me 🤓
There was a problem hiding this comment.
Also, just wanted to add, thanks so much for working on this!
|
Could you resolve the conflicts? I'll land as it's landable. |
|
@mcollina Conflicts resolved! Ready to finally land this before it gets stale again. 😅 EDIT: Apologies for the small formatting diffs. I was not intending to add more noise or change the style. |
resolves #104
Checklist
npm run testandnpm run benchmarkand the Code of conduct
Thoughts
I don't have a ton of experience in this testing framework, so I'm sure there's room for improvement on that front as well as accounting for more use-cases. Furthermore I also have a feeling that the implementation of the feature could be done more efficiently, but this is what I could come up with and get the tests passing 🤷
Definitely open to any and all suggestions to improve what I have so far.
Thanks!