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

brotli support #3037

Closed
patrickkettner opened this issue Jan 29, 2016 · 17 comments
Closed

brotli support #3037

patrickkettner opened this issue Jan 29, 2016 · 17 comments
Assignees
Labels
feature New functionality or improvement

Comments

@patrickkettner
Copy link
Contributor

It would be great to be able to server assets with brotli - especially if I could precompile and serve with lookupCompressed. In general it has a much better compression ratio than gzip.

@hueniverse
Copy link
Contributor

I don't know what exactly you are asking for.

@jamesdixon
Copy link

He appears to be asking for Hapi to support brotli compression in addition to gzip as well as allowing inert to lookup pre-compressed assets created with brotli. Maybe something that could be done in a plugin for the time being? Browser support appears to be nearly non-existent at this point....

@patrickkettner
Copy link
Contributor Author

Sorry for not being clearer. James is correct. Browser support is low, but it is a requirement for WOFF2 web fonts, and as such all browsers will be implementing it in the near future.

On Jan 30, 2016, at 5:52 PM, James Dixon [email protected] wrote:

He appears to be asking for Hapi to support brotli compression in addition to gzip as well as allowing inert to lookup pre-compressed assets created with brotli. Maybe something that could be done in a plugin for the time being? Browser support appears to be nearly non-existent at this point....


Reply to this email directly or view it on GitHub.

@hueniverse
Copy link
Contributor

I'll consider a PR.

@nrotta
Copy link
Contributor

nrotta commented May 24, 2016

@patrickkettner I will give it a try. I have an initial version (sort of) working and have already pushed the changes to Inert to my fork. Will push the initial hapi changes to my fork next.

Consider that I'm just starting with Hapi so my code might require lots of changes.

@nrotta
Copy link
Contributor

nrotta commented May 29, 2016

@patrickkettner I finished the work to support brotli on hapi. Looks like it works. Can you please check it out?

You have to use the following from my fork:

hapi (master branch): https://github.com/nrotta/hapijs
subtext (master branch): https://github.com/nrotta/subtext
inert (brotli branch): https://github.com/nrotta/inert

I have a failing test on inert so I cannot send the PRs yet. I will fix it over the next few days.

Thanks

@patrickkettner
Copy link
Contributor Author

I'd love to check it out! did you mean https://github.com/nrotta/hapi ? https://github.com/nrotta/hapijs doesn't exist

I only ask because nrotta/hapi doesn't contain any brotli commits form what I can see.

@nrotta
Copy link
Contributor

nrotta commented May 30, 2016

Yes, you are right. Sorry typo. The correct one is: https://github.com/nrotta/hapi

Let me know how it goes.

@patrickkettner
Copy link
Contributor Author

patrickkettner commented Jun 1, 2016

awesome work! It is not working for me, however. I believe this line has to be modified in order for brotli to be one of the encodings that is possible. or am I screwing something up?

@nrotta
Copy link
Contributor

nrotta commented Jun 1, 2016

I did modify that line: https://github.com/nrotta/hapi/blob/brotli/lib/request.js#L119

Try the brotli branch on my fork and let me know how it goes.

Btw, it took me a while to get my fork to be installed as a dependency in hapi. Not sure why but the updated code was not being installed. Once you install my hapi fork, you have to change the package.json to point to my fork of inert and subtext. And check the content of node_modules in your hapi install to ensure that the inert & subtext libs have my changes (you can look for 'brotli' on the code base).

Thanks

@AdriVanHoudt
Copy link
Contributor

@nrotta this is probably due to the shrinkwrap

@patrickkettner
Copy link
Contributor Author

@nrotta yeah, I found that out. npm link is how I usually get around sub dep issues

the latest change doesn't work. In this context, it would be br, not brotli. And since Accept.encodings() is finding the first supported compression type in the list, brotli wouldn't ever be reached behind gzip. If you move br to the front of the list (since it should be preferable, I imagine)

@nrotta
Copy link
Contributor

nrotta commented Jun 1, 2016

@AdriVanHoudt you are right. Thanks.

@patrickkettner you can set { 'accept-encoding': 'brotli' } in the header of the request and should work with brotli (instead of the default gzip). Btw, how are you testing? Can you send me the code to ensure that I'm not missing anything?

Regarding using br instead of brotli that could be changed but for what I read online there is no naming defined so I decide to go with brotli for the name of the encoding and br for the extension.

@patrickkettner
Copy link
Contributor Author

Regarding using br instead of brotli that could be changed but for what I read online there is no naming defined so I decide to go with brotli for the name of the encoding and br for the extension.

Chrome and Firefox send br. here is an example of Chrome's AE header

Accept-Encoding:gzip, deflate, sdch, br

I don't believe any one uses the string brotli in his context

@patrickkettner you can set { 'accept-encoding': 'brotli' } in the header of the request and should work with brotli (instead of the default gzip).

Sure, but no browser sends that header. they send something like the one I used above. gzip is always there as well

how are you testing

I don't have my laptop on me, but I just npm i nrotta/hapi, cd into node_modules/hapi, and npm i nrotta/subtext nrotta/inert#brotli

From there, I just used the hello world hapi example with a single route (and self signed SSL, since its a requirement for browser usage)

@nrotta
Copy link
Contributor

nrotta commented Jun 4, 2016

@patrickkettner you are right. I changed the encoding to 'br' and increased the priority of br over gzip.

Please check it out and let me know.

@hueniverse
Copy link
Contributor

Now possible via the new server methods decoder and encoder

@kanongil
Copy link
Contributor

Quick note that Inert has now been updated with multiple encoding support, and can serve pre-compressed brotli files with my newly published brotli encoder / decoder plugin.

@Marsup Marsup added feature New functionality or improvement and removed request labels Sep 20, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 21, 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

No branches or pull requests

7 participants