Add fetch calls #356
Add fetch calls #356
Conversation
|
PTAL, addressed all of your comments. The pending thing is the referrer being handwavy (in the case we do want to send it), while I figure out how to set it properly. |
| 1. [=request/window=] set to "no-window" | ||
| 1. [=request/service-workers mode=] set to "none" | ||
| 1. [=request/destination=] set to "identity" | ||
| 1. [=request/origin=] set to a unique [=opaque origin=] |
There was a problem hiding this comment.
what does this do? is this for the cache partitioning?
There was a problem hiding this comment.
It sets the origin of the request. The origin is 'user agent' but there is not such thing.
There was a problem hiding this comment.
Is this supposed to be referring to this [1]? [=opaque origin=] serializes to "null" rather than "user agent" right [2]?
Do you know what this is for?
[1] https://fetch.spec.whatwg.org/#concept-request-origin
[2] https://html.spec.whatwg.org/multipage/origin.html#concept-origin-opaque
There was a problem hiding this comment.
I'm not sure what origin is used when not using CORS but there is no such thing as 'user agent' origin in Fetch right now, so I think an opaque origin works best (this was Dominic's recommendation)
There was a problem hiding this comment.
If we don't know what this is for, why are we setting it? Is that a required field? Can we just leave it unset for now?
There was a problem hiding this comment.
I do not think so, because Fetch does not handle that properly. If request’s origin is "client", then set request’s origin to request’s client’s origin.
https://fetch.spec.whatwg.org/#ref-for-concept-request-origin%E2%91%A6:~:text=If%20request%E2%80%99s%20origin%20is%20%22client%22%2C%20then%20set%20request%E2%80%99s%20origin%20to%20request%E2%80%99s%20client%E2%80%99s%20origin.
|
Ready for more review, PTAL. |
There was a problem hiding this comment.
Looks generally good to me, but still feels like the parameters aren't quite right.
On a separate note, any chance we can get @domfarolino to take a pass at this too?
| 1. [=request/window=] set to "no-window" | ||
| 1. [=request/service-workers mode=] set to "none" | ||
| 1. [=request/destination=] set to "identity" | ||
| 1. [=request/origin=] set to a unique [=opaque origin=] |
There was a problem hiding this comment.
Is this supposed to be referring to this [1]? [=opaque origin=] serializes to "null" rather than "user agent" right [2]?
Do you know what this is for?
[1] https://fetch.spec.whatwg.org/#concept-request-origin
[2] https://html.spec.whatwg.org/multipage/origin.html#concept-origin-opaque
Can you look again but looking at the latest version?? |
|
Added |
| 1. [=request/window=] set to "no-window" | ||
| 1. [=request/service-workers mode=] set to "none" | ||
| 1. [=request/destination=] set to "identity" | ||
| 1. [=request/origin=] set to a unique [=opaque origin=] |
There was a problem hiding this comment.
If we don't know what this is for, why are we setting it? Is that a required field? Can we just leave it unset for now?
spec/index.bs
Outdated
| 1. [=request/destination=] set to "web-identity" | ||
| 1. [=request/origin=] set to a unique [=opaque origin=] | ||
| 1. [=request/header list=] set to a [=list=] containing a single [=header=] with | ||
| [=header/name=] set to "Accept" and [=header/value=] set to "JSON" |
There was a problem hiding this comment.
Again, shouldn't this be a mime type? Are you sure this isn't supposed to be text/json?
There was a problem hiding this comment.
Again, don't think so? MIME type is an object computed from headers[1] but header names and values are byte sequences [2]. I think it should be application/json based on the implementation.
[1] https://mimesniff.spec.whatwg.org/#mime-type
[2] https://fetch.spec.whatwg.org/#concept-header
| 1. [=request/window=] set to "no-window" | ||
| 1. [=request/service-workers mode=] set to "none" | ||
| 1. [=request/destination=] set to "identity" | ||
| 1. [=request/origin=] set to a unique [=opaque origin=] |
There was a problem hiding this comment.
I do not think so, because Fetch does not handle that properly. If request’s origin is "client", then set request’s origin to request’s client’s origin.
https://fetch.spec.whatwg.org/#ref-for-concept-request-origin%E2%91%A6:~:text=If%20request%E2%80%99s%20origin%20is%20%22client%22%2C%20then%20set%20request%E2%80%99s%20origin%20to%20request%E2%80%99s%20client%E2%80%99s%20origin.
spec/index.bs
Outdated
| 1. [=request/destination=] set to "web-identity" | ||
| 1. [=request/origin=] set to a unique [=opaque origin=] | ||
| 1. [=request/header list=] set to a [=list=] containing a single [=header=] with | ||
| [=header/name=] set to "Accept" and [=header/value=] set to "JSON" |
There was a problem hiding this comment.
Again, don't think so? MIME type is an object computed from headers[1] but header names and values are byte sequences [2]. I think it should be application/json based on the implementation.
[1] https://mimesniff.spec.whatwg.org/#mime-type
[2] https://fetch.spec.whatwg.org/#concept-header
|
LGTM I'll merge, and if there is anything pending we can patch separately. |
SHA: a38e359 Reason: push, by @samuelgoto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Relevant issues:
This PR adds the fetch calls to the other FedCM fetches.
Preview | Diff