feat: support for brotli

[bjohansebas]

The changes from #172 are brought in, with the exception of using the accept negotiation logic.

[bjohansebas]

It should use accepts, so this PR is not ready yet, and I don't have the option to turn it into a draft. I will soon open a PR in accepts to be able to use this logic

[sardeeplakhera]

@bjohansebas @blakeembrey Is there a tentative date for merging this?

[bjohansebas]

@sardeeplakhera I hope it can be merged soon, although there’s still some time needed to ensure that everything works correctly.

[sardeeplakhera]

Thanks, @bjohansebas! Do you think first week of December could be a realistic target for the release? Or is there anything else that might push it back further?

[bjohansebas]

@sardeeplakhera i think a new patch version for this package could be ready by December. Currently, this work is more like a proposal for version 1.8.0 (see #189)

[blakeembrey]

It may be a good opportunity to stop passing through opts blindly to each method and instead define it as brotliParams?

[bjohansebas]

I think in this case it makes sense to assign it this way, since the options for each compression method have not been separated yet, or maybe I misunderstood your idea.

https://nodejs.org/api/zlib.html#class-brotlioptions

[wesleytodd]

I think if we follow this suggestion it would also remove the need for the objectAssign addition right? because we could just manually enumerate the options for each compression method.

[bjohansebas]

If I'm understanding the idea correctly, the function would look something like this:

function compression (options) {
  var gzipOptions = {
    level: opts.level,
    memLevel: opts.memLevel,
    strategy: opts.strategy
  }

  var brotliOptions = {
    level: opts.level,
    memLevel: opts.memLevel,
    strategy: opts.strategy
    params: opts.brotliParams
  }
//...

  stream = method === 'gzip'
        ? zlib.createGzip(gzipOptions)
        : method === 'br'
          ? zlib.createBrotliCompress(brotliOptions)
          : zlib.createDeflate(gzipOptions)

[wesleytodd]

Don't create the objects unless you are going to use them, but yes in general this is what I was thinking.

[bjohansebas]

@wesleytodd, enumerating the options doesn't seem like a good idea to me, as it adds unnecessary maintenance to the package. I think adding the Brotli option separately should be sufficient for now.

[Fonger]

Thank you the maintenance team!
Glad to see it finally gets some progress.

[wesleytodd]

I am leaving this as "request changes" because I think we need to spend a bit more time to decide what changes should be landed from the comments before merging. That said, I don't actually consider any of my comments as blocking. If we get answers on the comments then ping me or just merge without more review from me.

[wesleytodd]

I think if we follow this suggestion it would also remove the need for the objectAssign addition right? because we could just manually enumerate the options for each compression method.

[bjohansebas]

hi @wesleytodd @blakeembrey, I've updated the options to avoid using object-assign, would love to hear your feedback

[blakeembrey]

I think this was better when using object-assign, now we would be mutating user input if it exists.

I also think optsBrotli is only a marginally better change, I'd probably do something more like this (if you want to go down this route):

var brotliOptions = {
  [zlib.constants.BROTLI_PARAM_QUALITY]: opts.brotliQuality,
};

I.e. the nesting of brotli: { quality: 4 } also seems reasonable, my main concern was that passing through as-is some of those option may actually interfere with the package working (like flush maybe?). I'm not too familiar with this, so trust your judgement on this.

[bjohansebas]

I will use Object.assign again to avoid directly mutating the object. Also, I prefer that developers pass the options directly, as it was done