feat: add default option#191
feat: add default option#191bjohansebas merged 10 commits intoexpressjs:masterfrom bjohansebas:add-default-encoding
Conversation
|
Hi @IamLizu, could you review this PR? |
|
Hey @bjohansebas 👋 Sure, I will read the relevant issues and then review this PR 👍 |
IamLizu
left a comment
IamLizu
left a comment
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 |
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 |
|
|
||
| // 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? |
wesleytodd
left a comment
wesleytodd
left a comment
There was a problem hiding this comment.
It looks to me like this was merged with a pending "change requested" review. Is that the case?
|
|
||
| // 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. |
3 participants
A new option is added to determine the default behavior when
Accept-Encodingis not sent by the client, defaulting to identity.closes #83