fetch
fetch copied to clipboard
whatwg
Editorial: Use controller for navigation redirect
Remove the existing navigation redirect, and allow the caller to use the controller to continue to the next redirect.
This simplifies the navigation HTML<->FETCH flow, as now there is
a single fetch params struct for the course of the entire navigation.
Up until now some data from fetch params would be lost, like timing info
and task desination, because navigation fetches create a brand new
fetch params struct. Now that data is retained, and the only thing that
HTML needs to do is call process the next redirect on the fetch controller.
- [ ] At least two implementers are interested (and none opposed):
- …
- …
- [ ] Tests are written and can be reviewed and commented upon at:
- …
- [ ] Implementation bugs are filed:
- Chrome: …
- Firefox: …
- Safari: …
- Deno (not for CORS changes): …
(See WHATWG Working Mode: Changes for more details.)
How does this work in terms of callbacks and such? The prior approach was that the algorithm would return a response. I'm not entirely sure that was workable, but I'm not sure what the new approach is.
The new approach is reviewable here: https://github.com/whatwg/html/pull/8097
How does this work in terms of callbacks and such? The prior approach was that the algorithm would return a response. I'm not entirely sure that was workable, but I'm not sure what the new approach is.
The new approach is reviewable here: whatwg/html#8097
Specifically, we don't need new callbacks because we don't replace fetch params. process response etc still work the same way, the only thing that changes is that instead of processing redirects automatically we have to call next from the outside.
How does this work in terms of callbacks and such? The prior approach was that the algorithm would return a response. I'm not entirely sure that was workable, but I'm not sure what the new approach is.
The new approach is reviewable here: whatwg/html#8097
Specifically, we don't need new callbacks because we don't replace
fetch params.process responseetc still work the same way, the only thing that changes is that instead of processing redirects automatically we have to callnextfrom the outside.
@annevk anything else missing here? Other changes in the HTML spec depend on this.
But how would HTML be notified that it needs to call "next"? Would it call that from "process response"? If so we need to document that "process response" can be invoked multiple times in this mode, no?
I did look at https://whatpr.org/html/8097/browsing-the-web.html but it doesn't actually fill in "process response" (although it does talk about it in a later step).
But how would HTML be notified that it needs to call "next"? Would it call that from "process response"? If so we need to document that "process response" can be invoked multiple times in this mode, no?
Yes, that's the idea, I can add a comment about this when describing process response.
I did look at https://whatpr.org/html/8097/browsing-the-web.html but it doesn't actually fill in "process response" (although it does talk about it in a later step).
process a navigate fetch, 15.5.2: process the next redirect and then waits for process a response. What's missing?
"process response" is an argument of the fetch algorithm, not something you can wait for.
"process response" is an argument of the fetch algorithm, not something you can wait for.
This was there in the HTML spec before and I didn't care to refactor it here... but I can if it makes a difference - perhaps process the next redirect can pass a new callback if it's more readable, but the behavior would be the same.
The existing setup is indeed broken. I'm wondering what our long(er) term perspective is here. I'm also worried that if we keep using the same fetch, fetch controller, and fetch params, we'll end up with weird timing and state situations on them. Typically when you've seen "process response" you wouldn't expect to start uploading again, for instance.
The existing setup is indeed broken. I'm wondering what our long(er) term perspective is here. I'm also worried that if we keep using the same fetch, fetch controller, and fetch params, we'll end up with weird timing and state situations on them. Typically when you've seen "process response" you wouldn't expect to start uploading again, for instance.
The long term perspective is that this is a single navigation fetch, that gets its redirects followed manually rather than automatically. So it makes sense that it shares the same fetch params and state. This is equivalent to how the chromium implementation works - there's a "follow" function that's specific to navigations rather than a whole new fetch. Does HTML upload data in the middle of a navigation redirect chain? Otherwise I don't see how the example applies.
When you do a <form> POST and you get one or more 308 redirects.
And the concern is that if we ever added state such as "final response" (which we have for timings!) it would potentially get a very different meaning here and might end up being invalidated upon "follow".
When you do a
<form>POST and you get one or more 308 redirects.And the concern is that if we ever added state such as "final response" (which we have for timings!) it would potentially get a very different meaning here and might end up being invalidated upon "follow".
Is the concern something that exists in the spec now? I don't see any reference to 308 in fetch/HTML. Would adding a new callback for this feel more future-proof? Other suggestions on how to unbreak this?
308 is a redirect status and 301/302/303 change the request method to GET. So it exists, but it's a bit harder to find what its implications are.
I don't think reusing the callback is the problem per se. We should document it's a bit weird for "navigate" though. I think the main problem might be all the state that we keep and don't reset (and some of it we need to keep and not reset, such as the URL list). Perhaps a follow-up issue to look at that more closely would be best for now.
I audited the state which is not reset. That is the fetch params:
- request: this is the thing we need to preserve, and that the existing spec does
- algorithms: preserving these will make things easier, in the event we ever fix HTML to use them properly
- task destination, cross-origin-isolated capability: we don't want to reset these
- controller: seems to work well. The timing info being carried over is probably a win.
- timing info: this seems like a win.
- preloaded response candidate: not applicable to navigations, will always be null.
I overall agree with the idea that, since we don't reset this state during non-"manual" redirects, this change is an improvement since it makes "manual" redirects more like non-"manual" ones.
I might tweak this PR to change "next redirect steps" to "next manual redirect steps", and "process the next redirect" to "process the next redirect manually" or even "process the next HTTP redirect manually".
@annevk, what do you think? Can we merge this, if I rebase it and maybe perform those renames?
