feat: add brotli support

patrickmichalina commented 5 years ago

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 commented 5 years ago

Node 10.16.0 (LTS) now has Brotli support too.

curbengh commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Is there anything missing in this PR to be merged?

dougwilson commented 5 years ago

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

garthenweb commented 5 years ago

@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 commented 4 years ago

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

gengjiawen commented 4 years ago

@dougwilson Can you take another look ?

KB1RMA commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

kubmir commented 4 years ago

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

dougwilson commented 4 years ago

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

KB1RMA commented 4 years ago

@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 commented 4 years ago

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

bjohansebas commented 2 weeks ago

Closed in favor of https://github.com/expressjs/compression/pull/194, @patrickmichalina thank you for the initiative. You can continue helping by reviewing https://github.com/expressjs/compression/pull/194 so we can launch it.

expressjs / compression

feat: add brotli support #156