mlisook / plastic-image

A Polymer 3.0 element which adds extra plasticity to <iron-image> with support for srcset and lazy loading
MIT License
30 stars 6 forks source link

webp url regex incorrectly assumes all webp urls end with .webp (so it fails e.g. if there are any query params) #38

Closed jab closed 6 years ago

jab commented 6 years ago

It looks like the webp regex only matches webp image urls that end with .webp (src):

_webpRegex: {
  type: RegExp,
  value: /.+\.webp$/i    // note the `$`
},

But this fails to match valid webp image urls like example.com/image.webp?v=1 or even example.com/image.webp?.

(click for aside) Technically, the filename extension part of a url isn't even what determines the filetype, rather the mimetype specified in the `Content-Type` header of the response does. So even a url like `example.com/avatar` with no extension may very well be a webp image. In many cases, this is even the best design, e.g. when the requester doesn't / shouldn't need to know what the image type is, and only expects that there's some image at the requested path, letting the server tell it what the type is (possibly generated / decided dynamically). This design is less common than query params, and maybe more complex to support, in which case this may bear considering separately.)

You could drop the `$` from the regex, but that would result in false positives like `/v2.webplatform.gif`. You could instead strip off any query string before testing against the regex, but let's back up a second. There's no reason ``’s webp detection needs to trade implicit magic for robustness. The `` API could simply let users explicitly tell the element what webp url regex to use, so `` doesn't have to either assume one size fits all, or do any additional url sniffing guesswork. Then, users have the flexibility to override the default regex in whatever way they need. e.g. A user may have no control over the filename extension in the url, and may only be able to indicate that a webp image is expected by adding (what would otherwise be a no-op) `webp=1` query param. In that case, the user could just append that query param to all their webp urls, specify the corresponding regex, and get perfect detection, e.g. ```html ``` What do you think? Hope this helps, and thank you for the great work on ``!
mlisook commented 6 years ago

Thank you for reporting this issue.

I really like your suggestion of allowing an override to the webp regex. It'll be quite straightforward to implement and document, so expect a new release sometime tomorrow.

mlisook commented 6 years ago

I've merged in a change, PR #39 , which adds the webpRegex property as a way to override the default regex, updated the readme to reflect that and added a couple of basic tests in test/webp.html

The new release is 1.0.14

jab commented 6 years ago

Tested the new web-regex API and looks like it's working great. Thanks for the quick fix and release!

mlisook commented 6 years ago

No problem. Thanks again for reporting the issue and a great way to fix it.