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

Support for static compression? #104

Closed
coreyfarrell opened this issue Apr 13, 2019 · 14 comments · Fixed by #158
Closed

Support for static compression? #104

coreyfarrell opened this issue Apr 13, 2019 · 14 comments · Fixed by #158
Labels
help wanted Help the community by contributing to this issue semver-minor Issue or PR that should land as semver minor

Comments

@coreyfarrell
Copy link
Contributor

Not sure if this is the right place to ask but is it possible to support static compressed files: For example if a request is received for http://localhost/bundle.js with Accept-Encoding: brotli, I want to serve the already compressed bundle.js.br instead of compressing bundle.js on the fly. Same if gzip compression is requested, I'd want fastify-static to send bundle.js.gz if it exists. For my goal if the pre-compressed file does not exist then I'd want to send the uncompressed file, lack of pre-compressed file would mean that the build system determined that the compressed file was actually larger.

Am I correct that for fastify-static to support this it would need to be supported by send? They have a PR to add this feature but it appears to have stalled.

@mcollina
Copy link
Member

That's actually a nice feature. Would you like to PR that?

@coreyfarrell
Copy link
Contributor Author

I suspect this would need to be implemented by send but it's discouraging that pillarjs/send#108 seems to have stalled out. What is your current feeling on #47?

@mcollina
Copy link
Member

At some point I started working on a refactor on send to decouple the generation of the headers from the actual pipe, check out https://github.com/mcollina/send/tree/new-api.

It's probably a significant chunk of work, but it would be amazing if you'd like to contribute.

@coreyfarrell
Copy link
Contributor Author

What is the target node.js of this branch? Your patch doesn't add a readable-stream to package.json as a dependency, I assume ^2.3.6 since that's what's installed by devDependencies?

Would you mind rebasing your branch against upstream master (it doesn't conflict but just to avoid any change I make causing a conflict). This way I can post PR's to your new-api branch.

@mcollina
Copy link
Member

I don’t have time to do any work there unfortunately, I would say take what is there (tests are not passing atm). It’s definitely not finished.. it’s something I coded on a plane.

@gerardmrk
Copy link

@zdragnar
Copy link

Has there been any traction on this? I'd be willing to pitch in if there's something actionable.

@mcollina
Copy link
Member

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@stale
Copy link

stale bot commented Oct 21, 2020

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.

@stale stale bot added the stale label Oct 21, 2020
@chrstntdd
Copy link
Contributor

chrstntdd commented Oct 30, 2020

I would be interested in contributing to this feature. Is there anything to keep in mind when implementing? The koa-static implementation seems straightforward, but I'm not sure if it would work the same for fastify.

Furthermore, what should the API be? I don't think pre-compressed assets are a common use case, but for those that are opting in, I'm thinking it should be possible to skip the exists() call to keep things quick. Can we safely make this assumption?

@stale stale bot removed the stale label Oct 30, 2020
@mcollina mcollina added semver-minor Issue or PR that should land as semver minor help wanted Help the community by contributing to this issue labels Oct 31, 2020
@mcollina
Copy link
Member

@chrstntdd sure thing! Let us know how you progress!

@coreyfarrell
Copy link
Contributor Author

I'm thinking it should be possible to skip the exists() call to keep things quick. Can we safely make this assumption?

@chrstntdd this is not true for every file. Cases exist where the compressed file would actually be larger than the original which often results in the build process skipping output of that one compressed file. This frequently happens with very small files, especially small image files. Probably could skip checking if the file exists and instead just try opening it with fallback on ENOENT.

@chrstntdd
Copy link
Contributor

Probably could skip checking if the file exists and instead just try opening it with fallback on ENOENT.

Ah. Thank you mentioning that. If I understand correctly then, the intended behavior would be to attempt to send a pre-compressed version of the asset (if they have opted into the behavior), defaulting to brotli first if we see the accept-encoding headers, falling back to gzip if accepted(?), then falling back to the uncompressed version?

@mcollina
Copy link
Member

mcollina commented Nov 2, 2020

Yes exactly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Help the community by contributing to this issue semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants