feat: add brotli support

[patrickmichalina]

createBrotliDecompress and createBrotliCompress are only available in Node v10.16.0 and above. If the platform you are running supports brotli and the client making a request accepts brotli encoding, this will return brotli encoded responses, otherwise it will fallback as before.

Added a config option brotli.

compression({
  ...,
  brotli: {
    enabled: false,
    zlib: { ... } // https://nodejs.org/api/zlib.html#zlib_class_brotlioptions
  }
})

[ghostganz]

Node 10.16.0 (LTS) now has Brotli support too.

[curbengh]

I think iltorb library is a better option for now, to be compatible with Node 8. Once Node 8 has reached EOL (planned 2019-12-31), then it can be replaced with native's.

Edit: #158 uses both zlib and iltorb.

[benwiggins]

Introducing a dependency that requires pre-compiled binaries and/or a gcc and python 2.7 toolchain just to bring a new feature to a non-active LTS branch that will be dead in 6 months doesn't seem "better" to me.

Node 8 is in maintenance, let it remain that way with the existing compression feature set.

[xquelard]

Is there anything missing in this PR to be merged?

[dougwilson]

#71 (comment)

[garthenweb]

@dougwilson To sum this up, a proper implementation for the options and it's documentation is missing here, correct? Test vise this PR looks already good to me (tests for options would be nice as well as soon as this is implemented).

So, regarding the options here some documentation.
BrotliOptions: https://nodejs.org/api/zlib.html#zlib_class_brotlioptions
ZlipOptions: https://nodejs.org/api/zlib.html#zlib_class_options

The only option that looks interchangeable to me is chunkSize. flush and finishFlush have the same name on both methods but they use different constants, so the meaning is probably not the same.
What would be the best option here? Should we maybe change the options completely so that we can separate brotli and zlib options from each other? Like

app.use(compression({
  filter: shouldCompress,
  brotli: {
    flush: zlib.constants.BROTLI_OPERATION_PROCESS,
    params: { [zlib.constants.BROTLI_PARAM_QUALITY]: 4 },
    ...
  },
  zlib: {
    flush: zlib.constants.Z_NO_FLUSH,
    memLevel: zlib.Z_DEFAULT_MEMLEVEL,
    ...
  }
}))

This would require a breaking change, but I would argue that adding this option is a breaking change anyhow if it should be activated by default (which I would prefer) and it would not be to hard to migrate.

What is your opinion on this? Am I missing a requirement from the comments in the other PR's/ issues?

I would love to get this into the library as it would hopefully have a positive effect on a lot of websites, and the PR already looks solid ❤️

[patrickmichalina]

@dougwilson struggling to reach 100% coverage with node environments that do not support brotli.

[gengjiawen]

@dougwilson Can you take another look ?

[KB1RMA]

Is this still being held up by being not being able to hit 100% coverage? Seems a shame for such a small yet great change which could make a big difference in a large number of sites!

[ryhinchey]

Is this still being held up by being not being able to hit 100% coverage? Seems a shame for such a small yet great change which could make a big difference in a large number of sites!

@KB1RMA I believe the biggest issue is confirming that a major version could be released for this. The existing api needs to change to add brotli support. See here #156 (comment)

@dougwilson are you open to releasing a new major version to this module with an updated api that will let people specify separate options for brotli and gzip?

[patrickmichalina]

@ryhinchey this PR actually breaks out brotli vs gzip settings. But a major version bump does make sense IMO.

compression({
  ...,
  brotli: {
    enabled: false, // default to false, unchanged behavior with current version
    zlib: { ... } // https://nodejs.org/api/zlib.html#zlib_class_brotlioptions
  }
})

Node environments without the new brotli feature are unable to execute a branch of code so the code coverage never reaches 100%. Anyway to work around this for that section of code?

[bjohansebas]

Closed in favor of #194, @patrickmichalina thank you for the initiative. You can continue helping by reviewing #194 so we can launch it.