whatwg / fetch

Fetch Standard
https://fetch.spec.whatwg.org/
Other
2.12k stars 333 forks source link

Clarify priority between blob.type and Content-Type in headers for request/response.blob() #1630

Closed CYBAI closed 1 year ago

CYBAI commented 1 year ago

Currently, the spec of blob() says

return a Blob whose contents are bytes and whose type attribute is this’s MIME type.

however, it doesn't mention when the body is a Blob with type, browsers should respect the blob's type or the Content-Type in headers.

there's a test in Chromium and WebKit.

 promise_test(function(t) { 
     var req = new Request('http://localhost/', 
                           {method: 'POST', 
                            body: new Blob([''], {type: 'Text/Plain'})}); 
     req.headers.set('Content-Type', 'Text/Html'); 
     return req.blob() 
       .then(function(blob) { 
           assert_equals(blob.type, 'text/plain'); 
           assert_equals(req.headers.get('Content-Type'), 'Text/Html'); 
         }); 
   }, 
   'MIME type unchanged if headers are modified after Request() constructor'); 

Chromium and WebKit are respecting blob's type so they could pass the test now. For Gecko, it fails to run the test because it would respect the headers' content-type.

With confirming with @annevk in https://github.com/WebKit/WebKit/pull/12376#issuecomment-1503175231, it seems the blob() should always respect the header's content-type as "this’s MIME type" wins. So, I wonder maybe we would need some notes (or algorithms in the blob()) to clarify more about blob's type when it exists in the spec?

Finally, if we should respect headers' Content-Type, I will fix the test to

-           assert_equals(blob.type, 'text/plain'); 
+           assert_equals(blob.type, 'text/html'); 

(As this test exists in Chromium and WebKit separately, I will try to make a WPT PR and help to remove the related one in Chromium and WebKit :pray:)

annevk commented 1 year ago

I think the main thing we want to do here is to change "this's MIME type" to be more obviously an algorithm invocation. I think that would address any potential confusion. But this would be an editorial change as in principle this already makes the relevant requirements.

CYBAI commented 1 year ago

I'd like to try working on the clarification. 🙋

I was wondering if https://fetch.spec.whatwg.org/#concept-body-mime-type is the right place to add the algorithm invocation 🤔 However, body doesn't have an associated headers.

Thus, maybe I should update https://fetch.spec.whatwg.org/#ref-for-concept-body-mime-type%E2%91%A2 and https://fetch.spec.whatwg.org/#ref-for-concept-body-mime-type%E2%91%A3 to run the algorithm instead?

annevk commented 1 year ago

That's great.

I think there are two approaches here:

  1. Renaming from "MIME type" to "get the MIME type" to add clarity. The concrete classes then continue to define the actual algorithm as they do today.
  2. Make it a standalone "get the MIME type" algorithm that takes and branches on a Request/Response object to do the right thing.

The standalone algorithm makes the mixin less generic, but that's not necessarily a bad thing I think. And it's probably the most clear.

CYBAI commented 1 year ago

Thank you! Sounds good to me! I will try with this approach.