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(server): implement brotli compression for server #17766

Closed
wants to merge 3 commits into from

Conversation

nicksrandall
Copy link

@nicksrandall nicksrandall commented Oct 10, 2020

Current Status: WIP

I'm new this repo so I need some help on figuring out how to run the tests and make sure that everything is working. Any guidance on how I should proceed would be very helpful.

Background

TL;DR

Next.js uses expressjs/compression to handle gzip compression for its HTTP server.

There has been several attempts (172, 173, 158) to add brotli compression support to that library and so far none have been successfully merged (for various reasons). One of the largest reasons why this has been a challenge is because that library requires that any code is supported all the way down to Node 0.8. Also, it uses the accepts npm package which (although it is spec compliant) does not generally do what users actually want with regard to how it determines which compression encoding to use.

I have re-written the expressjs/compression library in typescript and swapped out the accetps library for @hapi/accepts which handles this issue in a more preferred way. I've also extended the library to add support for brotli compression. See the code here: https://github.com/nicksrandall/compression

What is the problem that is trying to be solved?

The brotli compression algorithm is generally more efficient than gzip and now supported in recent versions of Node.js and in all major browsers. Http clients (like web browsers) can specify an "Accept-Encoding" header in their requests to share which compression algorithms they support and prefer. According to the spec, when two values have the same preference, the first value will be used (seems reasonable right?).

However, many browsers (including Google Chrome) send the "br" (brotli) value last even though it is generally preferred to the other algorithms (ie they send gzip, deflate, br instead of br, gzip, deflate). This causes the brotli compression algorithm to be de-prioritized and unused.

What can we do about it?

We can follow a well-established convention to deviate from the spec slightly and force the server to choose the "preferred" compression algorithm when the client has (basically) stated that it doesn't not explicitly prefer one algorithm over another.

So, for example, if we get an "Accept-Encoding" header value of gzip, deflate, br we will use br (brotli) because brotli is more efficient over gzip and their preference (set by client) is both 1 (the default value).

However, if we get gzip;q=0.8, br;q=0.1 (q is level of preference from 0 to 1 where 1 is most preferred) we will use 'gzip' because the client has explicitly stated that it is more preferred than brotli.

The @hapi/accepts library has this behavior by default so I'm using that as part of my compression library.

@nicksrandall
Copy link
Author

One concern I expect to be brought up is that my library is brand new and therefore untested. While my library has decent unit test coverage, It has not been "battle tested" in a production environment for an extended period of time. This exposes a certain level of risk for Next.js users.

That said, I'm not sure how to solve this problem. I'm open to handing over my repo to Vercel so that the community could have more control over the repo it self, or transferring the code into next.js core.

@nicksrandall
Copy link
Author

@Timer I'd love some direction on this PR... Can I get some feedback? Or instructions on what next steps should be?

@timneutkens
Copy link
Member

Sorry for the delay in getting back to you! We were busy with the Next.js conference and are starting to review PRs again (there's 190 open right now so it's quite the process).

Overall the approach looks good, however if we're going down the route of using another library, especially when not battle-tested, I'd prefer to bring the library into Next.js itself so that we can maintain it and provide compatibility with Next.js in the future. If you're open to that I'd be happy to accept a PR that adds it to the Next.js core 👍

@nicksrandall
Copy link
Author

I will close this PR and create a new one where I add the compression library to next.js core.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants