-
Notifications
You must be signed in to change notification settings - Fork 357
WIP: Add a mechanism to fetch to track progress #1843
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
base: main
Are you sure you want to change the base?
Conversation
callback FetchObserverCallback = undefined (FetchObserver requestObserver, FetchObserver responseObserver); | ||
|
||
[Exposed=(Window,Worker)] | ||
interface FetchObserver : EventTarget { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observer probably isn't the best name here?
<li><p>Return <var>p</var>. | ||
</ol> | ||
</div> | ||
|
||
TEMPORARY <dfn id=event-fetchobserver-progress event for=FetchObserver><code>progress</code></dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to define the event somewhere properly. (potentially taking the event definitions from XHR?)
@@ -9132,6 +9205,119 @@ done only by navigations). The <a>fetch controller</a> is also used to | |||
<a for="fetch controller">process the next manual redirect</a> for <a for=/>requests</a> with | |||
<a for=request>redirect mode</a> set to "<code>manual</code>". | |||
|
|||
<h2 id=interface-progressevent>Interface {{ProgressEvent}}</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken from XHR and should be removed from XHR in a corresponding PR.
|
||
<h3 id=firing-events-using-the-progressevent-interface>Firing events using the {{ProgressEvent}} interface</h3> | ||
|
||
<p>To <dfn id=concept-event-fire-progress>fire a progress event</dfn> named <var>e</var> at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should fetch really fire sync events? It would match XHR but I wonder if these should be queued instead?
|
||
<p><em>This section is non-normative.</em> | ||
|
||
<p>The suggested {{Event/type}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these events aren't used in Fetch so this table should probably be kept in the XHR spec.
</ol> | ||
|
||
<li> | ||
<p>Let <var>processResponse</var> given a <var>response</var> be these steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to do progress tracking for response we'll need to do it differently to XHR, XHR consumes the body inside of this algorithm but fetch obviously can't.
One idea is to wrap response's body with a wrapper readable stream that allows use to fire progress events when the body actually gets read. This way we don't consume anything to report progress, (we can make it so we only wrapper if there's a responseObserver with a progress event listeners).
FetchObserverCallback observer; | ||
}; | ||
|
||
callback FetchObserverCallback = undefined (FetchObserver requestObserver, FetchObserver responseObserver); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firefox has the below IDL. I split it so the observers still fire events named progress but it's maybe fine to keep it as a single eventtarget and just change the event names?
[Exposed=Window]
callback interface ObserverCallback {
undefined handleEvent(FetchObserver observer);
};
enum FetchState {
// Pending states
"requesting", "responding",
// Final states
"aborted", "errored", "complete"
};
[Exposed=(Window,Worker),
Pref="dom.fetchObserver.enabled"]
interface FetchObserver : EventTarget {
readonly attribute FetchState state;
// Events
attribute EventHandler onstatechange;
attribute EventHandler onrequestprogress;
attribute EventHandler onresponseprogress;
};
@@ -7558,6 +7559,14 @@ dictionary RequestInit { | |||
RequestDuplex duplex; | |||
RequestPriority priority; | |||
any window; // can only be set to null | |||
FetchObserverCallback observer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this actually be in RequestInit? I'm thinking it should probably be in a FetchInit subtype that inherits RequestInit? That way it isn't also passed to the Request constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we leave it in requestinit we'll have to add a way to track it across from Request into the fetch call if you do
const req = new Request(...);
fetch(req);
@@ -8584,10 +8593,67 @@ method steps are: | |||
https://github.com/whatwg/dom/issues/1031#issuecomment-1233206400 --> | |||
</ol> | |||
|
|||
<li><p>Let <var>hasUploadListeners</var> be false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be hasRequestListeners
An alternative design that could be considered is to just make Request and event target and fire progress events at the request object directly. Would require authors to construct a request attach a listener and then pass to fetch but might be okay? |
This is not ready for review this is WIP.
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff