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

Added support for brotli compression #3198

Closed
wants to merge 2 commits into from

Conversation

nrotta
Copy link
Contributor

@nrotta nrotta commented Jun 9, 2016

Added support for brotli compression. The required changes to subtext and inert have also been done and the PRs submitted for merging.

This PR implements the following: #3037

PS: this PR was not sent from the master branch as I already have a PR on that branch waiting to be merged

@nrotta
Copy link
Contributor Author

nrotta commented Jun 9, 2016

The CI will fail as it depends on the changes done on Inert and Subtext. Not sure how to set it up.

@@ -2310,7 +2310,7 @@ following options:
clean up the files generated by the framework. This can be done by keeping track
of which files are used (e.g. using the `request.app` object), and listening to
the server `'response'` event to perform any needed cleanup.
- `parse` - can be `true`, `false`, or `gunzip`; determines if the incoming payload is
- `parse` - can be `true`, `false`, `gunzip`, or `br`; determines if the incoming payload is
Copy link
Member

Choose a reason for hiding this comment

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

below where we describe that true and gunzip will gunzip as appropriate, you should also mention that br is for brotli support. also why abbreviate it? brotli is plenty short IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nlf it seems that this will be added as a plugin but in any case I updated the API doc (not sure if the added text is the best / most comprehensible though).

Regarding the use of br (instead of brotli), it is the naming scheme that it is being used: google/brotli#299

Copy link
Member

Choose a reason for hiding this comment

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

fair enough, silly decision on their part IMO, but if it's an established thing then we should probably stick with it

@nlf
Copy link
Member

nlf commented Jun 10, 2016

CI won't pass until the merges are done and published for your dependencies, you'll also have to update the shrinkwrap here to the correct versions

@hueniverse hueniverse self-assigned this Jun 10, 2016
@hueniverse
Copy link
Contributor

This is blocked until I figure out how to move it to to a plugin.

@hueniverse
Copy link
Contributor

@nrotta You can now create a new plugin that simply calls server.decoder() and server.encoder(). Let me know if you need help with the new features.

@hueniverse hueniverse closed this Aug 22, 2016
@hueniverse hueniverse added the feature New functionality or improvement label Aug 22, 2016
@kanongil
Copy link
Contributor

FYI, I took some time and put together a plugin for this, currently unpublished here: https://github.com/kanongil/brok

While it works, it would be much more useful if the request is exposed somehow when the encode function is called. This enables route level options for eg. configurable compression levels and mode.

@AdriVanHoudt
Copy link
Contributor

(@kanongil why not hotli? ^^)

@genediazjr
Copy link
Contributor

@AdriVanHoudt agree
@kanongil any chance to make it hotli?

@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants