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 |
| @@ -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
There was a problem hiding this comment.
Sorry but I don't follow your description? To me it looks like this value for the option (coming from the calling code) could just be dropped entirely.
There was a problem hiding this comment.
I haven't found a way to eliminate this check.
|
@wesleytodd Is there any change I need to make? |
| @@ -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.
Sorry but I don't follow your description? To me it looks like this value for the option (coming from the calling code) could just be dropped entirely.
|
Sorry for merging this with a change request, I now know I need to wait until the approval is there. |
A new option is added to determine the default behavior when
Accept-Encodingis not sent by the client, defaulting to identity.closes #83