Threshold of `false` seems confusing

[elmigranto]

Is threshold option being false means never compress no matter response size, or default threshold of 1 KiB, or something else?

// will compress any size
app.use(compression({threshold: 0}))

// returns JSON as plain text
app.use(compression({threshold: false}))

Makes sense to me that above two lines should be equivalent, but doesn't seem to be the case. Should either change this, or add more detailed description into README.

---

❤️ to everyone working on this 😊

[elmigranto]

Here's a check on whether default option should be used. It keeps threshold as false since false == null is never true:

https://github.com/expressjs/compression/blob/7ebf291087d69156df4b086e2020e660a1dffb17/index.js#L55-L57

And here's a check resulting in never compressing anything when threshold is false since Number(false) returns 0 and compression would only be applied to responses of negative size (which is never):

https://github.com/expressjs/compression/blob/7ebf291087d69156df4b086e2020e660a1dffb17/index.js#L156-L160

---

Not sure if this is a long-forgotten thing in the README, actual bug or intended behaviour. But I am sure it is very surprising that passing {threshold: false} option disables compression entirely.

[elmigranto]

Here's a README section I am talking about https://github.com/expressjs/compression/blob/7ebf291087d69156df4b086e2020e660a1dffb17/README.md#threshold.

[elmigranto]

I would proropse we don't do any length-checking and always compress when false was explicitly passed by the user, but again, not sure what was intended behaviour here. Alternatively, we can throw on anything other than non-negative integers or valid byte strings removing the possibility to pass false entirely.

[elmigranto]

To be clear, this is debug output I get for {threshold: false} on smallish response:

compression no compression: size below threshold +0ms

And here's for {threshold: 0}:

compression gzip compression +0ms

[elmigranto]

Oh, my bad, {threshold: false} compresses things greather than 1 KiB:

res.json({settings: app.settings, hugeString: '0'.repeat(1000)});

results in

compression gzip compression +0ms

[dougwilson]

Hi @elmigranto sorry you were having trouble using the module. At first when I read your issue I was confused because I didn't even know that false was a value allowed for the threshold option at all. I looked at it was indeed listed in the docs as a value, though when I checked in the tests there is no tests for such a value.

The implementation clearly does not support the value, as you observed.

I took a look back at the history of this project, various points in time of the implementation show that false has never been a supported value and looking at how it got added in the readme, it looks like a mistake that it got there at all.

I'm going to fix the readme for the time being.

I never thought about false being a possible value, but I think that makes sense I suppose. If you are interested in seeing false be a supported value, you're welcome to make a pull request to support it, of course 👍

[elmigranto]

0 does the trick for me, so I think simply dropping that bit from the docs is just fine. Thanks, Doug.