143: Fix issue when specifying a false condition for using webp #144
Conversation
`args.webp` is a string by default, so using a conditional with a string in it will always be true unless it is an empty string. This gives a very odd results to have to use `webp=` in order to force using a jpeg. Converting the parameter to a boolean fixes potential issues where future conditionals are just using `(args.webp)` where any string > 0 characters, is equal to true. In addition a conditional which doesn’t consider the datatype allows most strings to be false and `1` to be true as defined in the docs.
There was a problem hiding this comment.
This allows the tachyon server to support boolean values for webp expressed as "0" or "1", which matches the documentation at https://github.com/humanmade/tachyon/blob/d82520f79a43070fd99ab6587bca1b061cabbea4/docs/using.md
👍 I think this is more logical than having to use an empty string to disable webp formatting.
|
@joehoyle what would be the next steps to get this merged, as we have a client awaiting this update 😄 |
| @@ -46,6 +46,9 @@ http.createServer( function( request, response ) { | |||
| const args = params.query || {}; | |||
| if ( typeof args.webp === 'undefined' ) { | |||
| args.webp = !!( request.headers && request.headers['accept'] && request.headers['accept'].match( 'image/webp' ) ); | |||
| } else { | |||
| // args.webp will always be a string at this point, so lets convert it to a boolean to not break future conditionals. | |||
| args.webp = (args.webp == true); | |||
There was a problem hiding this comment.
Sorry folks I missed this.
So, if we expect args.webp == true to return false in the case of "0" being passed, then I don't see why the later checks would also be failing. Should this not be something more like args.webp == "true" || args.webp == "0".
There was a problem hiding this comment.
The idea was to not break the existing workaround but also support using a real truthy value.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok, my bad, this I did not expect! I assumed that if ( "true" == true ) is identical to if ( "true" ) but, apparently that isn't the case.
So, just to confirm: we're expecting args.webp === "0". Currently if ( args.webp ) // when args.webp = "0" evaluates to true, so there's effectively no way to turn off webp. if ( args.webp == true ) // when args.webp = "0" evaluates to false. I don't know if it's because it's a Friday but this is totally breaking my brain :P
|
Also I'm just curious as to the use case here, what's the rationale for explicitly switching off webp? |
|
@joehoyle We have a client that has the desire to download images and wants them in a JPG format. |
|
I can't speak for @smccafferty and can't really speak for Alexis either, but I do know @ajvillegas worked on a recent feature where a client wanted to make full-resolution initial images available for download. JPEG is still easier to work with locally than WebP, so for now we have disabled tachyon URLs on those download links to enable users to get the actual image with minimum fuss, where we'd rather have it go through Tachyon but reliably return the original file type. |
|
Both @smccafferty and @kadamwhite are correct. |
args.webpis a string by default, so using it in a conditional will always be true unless it is an empty string. This gives a very odd result to have to usewebp=in order to force using a jpeg. Converting the parameter to a boolean fixes potential issues where future conditionals are just using(args.webp)where any string > 0 characters, is equal to true. In addition a conditional which doesn’t consider the datatype allows most strings to be false and1to be true as defined in the docs.