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

make compressibleTypes user settable so custom types can be compressed #32

Merged
merged 2 commits into from
Sep 8, 2018

Conversation

tobinbradley
Copy link
Contributor

The itch I'm scratching is I couldn't compress a protobuf content type, as it wasn't in mime-db or in the compressibleTypes constant. I was going to add it to compressibleTypes, but that didn't seem like a sustainable solution, so I made the compressibleTypes regex user configurable.

Since the types that were in compressibleTypes are also in mime-db, I'm guessing this is used to short circuit the mime-db check for performance. Figured overwriting compressibleTypes this way would be more performant than adding an additional regex check for a user passed expression (i.e. compressibleTypes.test(type) || someuserstuff.test(type)).

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.

Thanks! Would you mind adding a unit test?

@tobinbradley
Copy link
Contributor Author

I added 3 unit tests - check to see if it compresses if custom content type matches, that it doesn't compress if custom content type doesn't match, and that it won't compress (or break) if somebody passes in not-regex. Also cleaned up the docs for that section a bit.

Kudos on your test suite. I'm new to unit testing, but the way you did it with fastify.inject was really easy to pick up.

@tobinbradley
Copy link
Contributor Author

I should also mention the unit tests for custom content type are using junk values - I didn't want to use anything that might become a real content type some day.

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

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.

LGTM

@delvedor delvedor merged commit eed8fb8 into fastify:master Sep 8, 2018
@delvedor
Copy link
Member

delvedor commented Sep 8, 2018

Sorry for the delay!
Landed in v0.7.0 :)

@tobinbradley
Copy link
Contributor Author

Awesome, and thanks for the great project!

@tobinbradley tobinbradley deleted the compress-types-option branch September 10, 2018 20:31
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.

3 participants