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

feat: add support for serving statically compressed files #158

Merged
merged 16 commits into from
May 17, 2021

Conversation

chrstntdd
Copy link
Contributor

@chrstntdd chrstntdd commented Nov 11, 2020

resolves #104

Checklist

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!

@chrstntdd chrstntdd changed the title draft feat: add support for serving statically compressed files (resolves #104) draft feat: add support for serving statically compressed files Nov 11, 2020
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! Could you add a test with wildard: false? thanks

@chrstntdd
Copy link
Contributor Author

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?

@mcollina
Copy link
Member

That'd be enough!

@chrstntdd
Copy link
Contributor Author

Resolved everything so far. Should we get more eyes on this?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@chrstntdd
Copy link
Contributor Author

Awesome! Thank you for the patience with the review @mcollina. What are the next steps?

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@chrstntdd
Copy link
Contributor Author

@delvedor I think it could be useful, but I am not certain it would be worth the complexity.
How best would that option for a separate directory be supported while also allowing for this simpler case where the compressed file is a sibling?

const options: FastifyStaticOptions = {
  // ...
  preCompressed?: true | PathLike
}

I tried my best to get the same behavior from koa-send.

@chrstntdd
Copy link
Contributor Author

I started trying out this branch in a side project and I'm finding that with the new behavior I also have to implement setHeaders to set the correct content-type. The docs specify that the default is application/octet-stream, but without preCompressed enabled, the content-type is correctly set internally.
Any thoughts about why this might be?

@stale
Copy link

stale bot commented Dec 25, 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 Dec 25, 2020
Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some consideration about the header evaluation.

I would add the separate compressed path feature in a follow-up pr

@stale stale bot removed stale labels Dec 26, 2020
@mcollina
Copy link
Member

@delvedor @Eomm could you take a look?

@Eomm
Copy link
Member

Eomm commented Mar 24, 2021

Could you resolve the conflics?

I think it is necessary to address this use case for a safe feature

@chrstntdd chrstntdd requested review from delvedor and Eomm April 15, 2021 00:37
@chrstntdd
Copy link
Contributor Author

Hey folks. I updated this PR with the latest version and added encoding-negotiator per the suggestion. Ran into some auto-formatting troubles along the way - apologies for the extra noise but everything seems in order.

I didn't add any tests with the weighted encoding format since I think all that handled by encoding-negotiator itself. Let me know if there's more I can do to get this through.

Thank you all for your patience!

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

@delvedor @Eomm wdyt?

@@ -289,6 +289,24 @@ GET .../public/index
GET .../public/index.json
```

#### `preCompressed`

Default: `false`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤓

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, just wanted to add, thanks so much for working on this!

@mcollina
Copy link
Member

Could you resolve the conflicts? I'll land as it's landable.

@chrstntdd
Copy link
Contributor Author

chrstntdd commented May 14, 2021

@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.

@mcollina mcollina changed the title draft feat: add support for serving statically compressed files feat: add support for serving statically compressed files May 16, 2021
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

@delvedor @Eomm any other concerns before landing?

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.

Support for static compression?
6 participants