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: Adding support for brolti #173

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ The following compression codings are supported:

- deflate
- gzip
- br (brotli)

**Note** Brotli is supported only since Node.js versions v11.7.0 and v10.16.0.

## Install

Expand Down Expand Up @@ -44,7 +47,8 @@ as compressing will transform the body.

`compression()` accepts these properties in the options object. In addition to
those listed below, [zlib](http://nodejs.org/api/zlib.html) options may be
passed in to the options object.
passed in to the options object or
[brotli](https://nodejs.org/api/zlib.html#zlib_class_brotlioptions) options.

##### chunkSize

Expand Down Expand Up @@ -101,6 +105,20 @@ The default value is `zlib.Z_DEFAULT_MEMLEVEL`, or `8`.
See [Node.js documentation](http://nodejs.org/api/zlib.html#zlib_memory_usage_tuning)
regarding the usage.

##### params *(brotli only)* - [key-value object containing indexed Brotli parameters](https://nodejs.org/api/zlib.html#zlib_brotli_constants)

- `zlib.constants.BROTLI_PARAM_MODE`
- `zlib.constants.BROTLI_MODE_GENERIC` (default)
- `zlib.constants.BROTLI_MODE_TEXT`, adjusted for UTF-8 text
- `zlib.constants.BROTLI_MODE_FONT`, adjusted for WOFF 2.0 fonts
- `zlib.constants.BROTLI_PARAM_QUALITY`
- Ranges from `zlib.constants.BROTLI_MIN_QUALITY` to
`zlib.constants.BROTLI_MAX_QUALITY`, with a default of
`4` (which is not node's default but the most optimal).

Note that here the default is set to compression level 4. This is a balanced setting with a very good speed and a very good
compression ratio.

##### strategy

This is used to tune the compression algorithm. This value only affects the
Expand Down Expand Up @@ -141,6 +159,14 @@ The default value is `zlib.Z_DEFAULT_WINDOWBITS`, or `15`.
See [Node.js documentation](http://nodejs.org/api/zlib.html#zlib_memory_usage_tuning)
regarding the usage.

##### preferClient

This library by default will prioritize the optimal compression encoding algorithm if the client has
given more than one encoding the same explicit "quality". This is generally what users want and it
follows an established pattern used by other popular web servers. If you would like to opt-out of this
behavior (and therefore be more spec compliant), you can set `preferClient` to "true" and when
two encodings have the same "quality" the first one will be used.

#### .filter

The default `filter` function. This is used to construct a custom filter
Expand Down
89 changes: 81 additions & 8 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ var debug = require('debug')('compression')
var onHeaders = require('on-headers')
var vary = require('vary')
var zlib = require('zlib')
var objectAssign = require('object-assign')

/**
* Module exports.
Expand All @@ -30,6 +31,17 @@ var zlib = require('zlib')
module.exports = compression
module.exports.filter = shouldCompress

/**
* @const
* whether current node version has brotli support
*/
var hasBrotliSupport = 'createBrotliCompress' in zlib

var preferredEncodings = ['gzip', 'deflate', 'identity']
if (hasBrotliSupport) {
preferredEncodings.unshift('br')
}

/**
* Module variables.
* @private
Expand All @@ -48,6 +60,17 @@ var cacheControlNoTransformRegExp = /(?:^|,)\s*?no-transform\s*?(?:,|$)/
function compression (options) {
var opts = options || {}

if (hasBrotliSupport) {
// set the default level to a reasonable value with balanced speed/ratio
if (opts.params === undefined) {
opts.params = {}
}

if (opts.params[zlib.constants.BROTLI_PARAM_QUALITY] === undefined) {
opts.params[zlib.constants.BROTLI_PARAM_QUALITY] = 4
}
}

// options
var filter = opts.filter || shouldCompress
var threshold = bytes.parse(opts.threshold)
Expand Down Expand Up @@ -173,14 +196,15 @@ function compression (options) {
return
}

// compression method
var accept = accepts(req)
var method = accept.encoding(['gzip', 'deflate', 'identity'])
// force proper priorization
var headers = objectAssign({}, req.headers, options.prioritizeClient ? null : { 'accept-encoding': prioritize(req.headers['accept-encoding']) })

// we really don't prefer deflate
if (method === 'deflate' && accept.encoding(['gzip'])) {
method = accept.encoding(['gzip', 'identity'])
}
// the accepts function takes in a request object but only reads the headers
// So, to save a bit of memory, we send an object with only the headers propery
// this way we don't have to clone the entire request
var accept = accepts({ headers: headers })
// compression method
var method = accept.encoding(preferredEncodings)

// negotiation failed
if (!method || method === 'identity') {
Expand All @@ -192,7 +216,9 @@ function compression (options) {
debug('%s compression', method)
stream = method === 'gzip'
? zlib.createGzip(opts)
: zlib.createDeflate(opts)
: method === 'br'
? zlib.createBrotliCompress(opts)
: zlib.createDeflate(opts)

// add buffered listeners to stream
addListeners(stream, stream.on, listeners)
Expand Down Expand Up @@ -286,3 +312,50 @@ function toBuffer (chunk, encoding) {
? Buffer.from(chunk, encoding)
: chunk
}

/**
* Most browsers send "br" (brotli) as the last value in
* in the 'Accept-Encoding' header which causes it to be
* deprioritized according to the spec.
*
* This is typically not what end users actually want so here
* we force the "br" (brotli) value to first in the list so that
* it will get properly prioritized and used.
*
* It's worth noting that although this is not "spec compliant",
* we belive it follows a well-established convention.
*
* @private
*/
function prioritize (str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this, it seems like this can be an option added to the accept negotiation logic directly? Like an option to prefer server order over client order? I think that is effectively what this is doing, or am I misunderstanding?

Copy link
Author

Choose a reason for hiding this comment

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

Great idea. I have added a priorizeClient option that will allow users to opt-out of this functionality.

For what it's worth, we were already using this behavior because we were prioritizing GZIP over deflate even when deflate came first. As such, I think "opt-out" vs "opt-in" is acceptable in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, I think I didn't explain very well my thought on this; it wasn't to add a parameter here to this library's API, but rather to the accepts API. As far as adding this as a parameter to this module's API, I'm not sure if it's actually necessary, though if it is going to be surfaced as a switch, I would kind of expect such a switch to default to the spec compliant method. But again, I'm not sure if this actually needs to be a switch for the user; a better user switch would actually be to turn on/off each individual compression method, but that would be out of scope for this PR.

Copy link
Author

@nicksrandall nicksrandall Oct 8, 2020

Choose a reason for hiding this comment

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

I see what you are saying but we were already out of spec because we were prioritizing gzip over deflate even when deflate came first. The option to be fully spec compliant would be a new feature for this library.

I also will argue that most people do not want the spec compliant behavior and they will prefer the default option that we have here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are miss understanding what I am saying on this comment again, I'm really sorry. I must not be good at communicating my idea here through writing and that is my bad. I tried twice and it wasn't successful :( Perhaps if you desiure to move this forward would you be willing to do some kind of voice call?

Copy link
Member

Choose a reason for hiding this comment

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

@wesleytodd This was already included in version 2 of accepts, right?

return str
nicksrandall marked this conversation as resolved.
Show resolved Hide resolved
.split(',')
.sort(sortEncodings)
.join(',')
}

/**
* Sort compression encodings in order of our preference:
* br > gzip > deflate
*
* @private
*/
function sortEncodings (a, b) {
var al = a.toLowerCase()
var bl = b.toLowerCase()
if (al.indexOf('br') >= 0) {
return -1
}
if (al.indexOf('gzip') >= 0) {
return bl.indexOf('br') >= 0 ? 1 : -1
}
// we need these inverse rules to fix a stable sort bug
// found in node 10.x
if (bl.indexOf('br') >= 0) {
return 1
}
if (bl.indexOf('gzip') >= 0) {
return 1
}
return 0
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"bytes": "3.0.0",
"compressible": "~2.0.17",
"debug": "2.6.9",
"object-assign": "4.1.1",
"on-headers": "~1.0.2",
"safe-buffer": "5.2.0",
"vary": "~1.1.2"
Expand Down
Loading