Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic Accept header value */* and Service Workers #634

Closed
tomayac opened this issue Nov 20, 2017 · 13 comments
Closed

Generic Accept header value */* and Service Workers #634

tomayac opened this issue Nov 20, 2017 · 13 comments

Comments

@tomayac
Copy link
Contributor

tomayac commented Nov 20, 2017

Generic Accept headers like, for example, */* for images in Firefox make dealing with such requests in a Service Worker context hard and encourage relying on file extension sniffing, especially as there is no consistency among how browsers deal with setting this value.

This issue is somewhat related to #521 for dynamically created Requests , but also covers static fetches coming from img, script, link, etc.

@wanderview suggested to open an Issue with Fetch in order to discuss this.

@tomayac tomayac changed the title Generic Accept header value */* and Service Workers Generic Accept header value */* and Service Workers Nov 20, 2017
@jakearchibald
Copy link
Collaborator

Does request.destination solve this issue? https://fetch.spec.whatwg.org/#requestdestination

@annevk
Copy link
Member

annevk commented Nov 20, 2017

Currently https://fetch.spec.whatwg.org/#concept-fetch recommends a particular Accept header for image requests. If browsers don't have that it would be a bug. (As is not supporting destination.)

@wanderview
Copy link
Member

Note, we send / for image requests, but not we get some other accept headers wrong. I have this bug open to align with the spec:

https://bugzilla.mozilla.org/show_bug.cgi?id=1417463

@tomayac
Copy link
Contributor Author

tomayac commented Nov 20, 2017

I guess request.destination would solve this, iff ("ff") universally available. For the Accept header discussion, I found Firefox's https://bugzilla.mozilla.org/show_bug.cgi?id=1249474 quite enlightening, where the move to */* was motivated. Not sure where this leaves us (and as I am new here, I definitely missed a ton of the discussions that were had, so sorry for any comment that for the people longer around may seem dumb).

@annevk
Copy link
Member

annevk commented Nov 20, 2017

@tomayac please don't worry about asking questions. They're essential. (If we keep getting certain questions a lot, that's probably a reason to start writing some documentation.)

I think the main takeaway here is that browsers need to fix various issues. An active thing that you or someone else could take up is to ensure there is web-platform-tests test coverage and appropriate browser bugs are filed (and maybe linked from this issue for completeness). I don't think anything here warrants a change to the standard right now.

@tomayac
Copy link
Contributor Author

tomayac commented Nov 21, 2017

Thanks for the kind words, @annevk!

Looking for bugs to track, so far I found two:

There doesn't seem to be anything yet for Edge or WebKit, but I might just not be digging deeply enough. Anyone aware of bugs there, or for other browsers?

@wanderview
Copy link
Member

FWIW, I think destination implementation has been slow for a couple reasons:

  1. We had not heard developers asking for it yet with use cases. So this feedback is good.
  2. The spec changed a lot from the initial RequestContext spec, so the feature got put on the back burner while that all happened. It seems pretty stable now, though.
  3. There aren't really any WPT tests for it yet which makes first implementation more work. It means whoever does this first will probably have more work to not only write the test, but ensure the spec works as intended.

Anyway, I'll see if we can bump up the priority of the firefox implementation. It probably won't be immediate, but maybe we can do it in Q1 of 2018.

@tomayac
Copy link
Contributor Author

tomayac commented Dec 21, 2017

Really happy about Safari Technology Preview's implementation of request.destination:

screen shot 2017-12-21 at 10 28 30
screen shot 2017-12-21 at 10 28 21
screen shot 2017-12-21 at 10 27 52

🎉 Well done, team WebKit!

@yoavweiss
Copy link
Collaborator

Request.destination shipped in Chromium a few months back.
WPT results don't look too bright anywhere else though... (and also in Chromium, they're not perfect)

@wanderview
Copy link
Member

WPT results don't look too bright anywhere else though... (and also in Chromium, they're not perfect)

Looks better with the experimental branches:

https://wpt.fyi/results/fetch/api/request/destination?label=experimental

Firefox support will ship in stable 61 next week, I believe.

@nhoizey
Copy link

nhoizey commented Aug 6, 2018

Hi everyone. I'm not seeking advice or any solution for my current issue, but my use case might help motivate (if needed) implementation of request.destination:

I've got the issue of resources loaded via a Service Worker that have a .gif extension, but actually a video content type.

The reason is Cloudinary's Fetch API allows me to convert an animated GIF to a video with parameters in the middle of the URL, but the URL's end is the source GIF file. Details in my recent blog post: Using Cloudinary’s Fetch API to convert an animated GIF to a video.

request.destination is not ready everywhere yet, so I wonder how I should implement my caching strategy to cache actual GIFs but not these "strange" GIFs that are actually videos.

I used to filter URLs with the images extensions (because of the generic */* Accept header), but now I have to accept .gif while removing videos.

I added a test for the presence of /(f_mp4|f_webm)/ in the URL (that's the Cloudinary way to transform into another format), but this isn't very satisfactory.

request.destination would be perfect to solve my issue.

Update: I see only green "PASS" on the "Check destination attribute" row in these WPT results, but when I try this test (thanks @yoavweiss for the link) in my Safari 11.1.2, it fails.

image

@annevk
Copy link
Member

annevk commented Mar 7, 2019

Given there's tests and various browser bugs I'm going to close this. If there's something I missed though please let me know.

@annevk annevk closed this as completed Mar 7, 2019
@wanderview
Copy link
Member

Note, @bakulf also recently fixed the firefox accept header bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1417463

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants