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]

https://github.com/expressjs/compression/issues/71#issuecomment-519195492

[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 https://github.com/expressjs/compression/pull/156#issuecomment-529698666

@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?

[adityapatadia]

This is really a plight that such important change is not released. We have released a new package with Brotli feature: https://github.com/gumlet/express-compression

We will maintain that package.

[dougwilson]

@adityapatadia : https://www.npmjs.com/package/shrink-ray

[kubmir]

What is the plan for releasing this feature? I think brotli support would be great ... @dougwilson

[dougwilson]

@kubmir: https://www.npmjs.com/package/shrink-ray

[KB1RMA]

@kubmir: https://www.npmjs.com/package/shrink-ray

@dougwilson Sorry to be frank, but shrink-ray is a ridiculous reply/solution. I'm really struggling to understand why there's so much pushback on releasing this.

[dougwilson]

@KB1RMA I have provided information on what the issue is with this PR multiple times, even even specific replies to you. I am temporary locking this thread (whcih does not prevent pushing to this PR to address issues) as a reply like that is not constructive.

I was providing an off topic reply to those folks specifically for a fork that does it today.