feat: add default option #191
Conversation
bjohansebas
force-pushed
the
add-default-encoding
branch
from
37d0789
to
8880f40
Compare
bjohansebas
force-pushed
the
add-default-encoding
branch
from
8880f40
to
db89408
Compare
bjohansebas
force-pushed
the
add-default-encoding
branch
from
db89408
to
5d6a827
Compare
|
Hi @IamLizu, could you review this PR? |
|
Hey @bjohansebas 👋 Sure, I will read the relevant issues and then review this PR 👍 |
There was a problem hiding this comment.
Hi @bjohansebas 👋
I'm generally 👍 of the concept of introducing an enforcement mechanism, as discussed in #83.
However, would you mind renaming the variable defaultEnconfig to something that doesn't conflict with zlib.createGzip or zlib.createDeflate? It seems this conflict may have led to the addition of the following block:
if (opts.defaultEncoding) {
opts.defaultEncoding = undefined
}I suggest enforceEncoding, more on this below. Renaming the variable would help keep the code clean, and it’s important that we avoid such conflicts.
Thanks so much for your understanding and effort!
|
@IamLizu Good catch |
bjohansebas
force-pushed
the
add-default-encoding
branch
from
073f59f
to
7a3f9ef
Compare
There was a problem hiding this comment.
LGTM 👍
There's something I should address. As we know, content negotiation is handled by negotiator here, and it is indeed negotiator's responsibility to choose the best method.
Maybe we are interfering with negotiator's responsibilities?
cc: @expressjs/express-tc
|
@UlisesGascon ping |
index.js
Outdated
| @@ -177,6 +180,11 @@ function compression (options) { | |||
| var negotiator = new Negotiator(req) | |||
| var method = negotiator.encoding(['gzip', 'deflate', 'identity'], ['gzip']) | |||
|
|
|||
| // if no method is found, use the default encoding | |||
| if (encodingSupported.indexOf(enforceEncoding) !== -1 && !req.headers['accept-encoding']) { | |||
There was a problem hiding this comment.
nit: can you change the order of this? We should not check if the option is valid unless we are going to use it.
| @@ -177,6 +180,11 @@ function compression (options) { | |||
| var negotiator = new Negotiator(req) | |||
| var method = negotiator.encoding(['gzip', 'deflate', 'identity'], ['gzip']) | |||
|
|
|||
| // if no method is found, use the default encoding | |||
| if (encodingSupported.indexOf(enforceEncoding) !== -1 && !req.headers['accept-encoding']) { | |||
| method = enforceEncoding === '*' ? 'gzip' : enforceEncoding | |||
There was a problem hiding this comment.
I dont see * referenced anywhere but here. Can you describe why this is necessary when you could drop this check and just have folks pass gzip directly for this?
There was a problem hiding this comment.
If * is not checked and changed, the deflate encoding would be applied as it’s the last one in the check (
Lines 196 to 198 in 66c0cb7
A new option is added to determine the default behavior when
Accept-Encodingis not sent by the client, defaulting to identity.closes #83