Re: [w3c/ServiceWorker] Have "Handle Fetch" return the request body in addition to response (#1563)

@annevk commented on this pull request.

I think this mostly looks good and I think I also misunderstood some things initially. Left suggestions for improvements.

>        1. Assert: |request|'s [=request/destination=] is not "<code>serviceworker</code>".
       1. If |request|'s [=request/destination=] is either "<code>embed</code>" or "<code>object</code>", then:
-          1. Return null.
+          1. Return null and |requestBody|.

How is this case not impacted? Is that because there's not even a fetch event for these? Maybe we should return null and null there for clarity to indicate nothing could have changed?

>            1. If |request|'s [=request/destination=] is not {{RequestDestination/"report"}}, set |reservedClient|'s <a>active service worker</a> to |registration|'s <a>active worker</a>.
           1. If |request| is a [=navigation request=], |registration|'s [=navigation preload enabled flag=] is set, |request|'s [=request/method=] is \`<code>GET</code>\`, and |registration|'s [=active worker=]'s <a>set of event types to handle</a> [=set/contains=] <code>fetch</code>, then:
 
               Note: If the above is true except |registration|'s [=active worker=]'s <a>set of event types to handle</a> **does not** contain <code>fetch</code>, then the user agent may wish to show a console warning, as the developer's intent isn't clear.
 
-              1. Let |preloadRequest| be the result of [=request/cloning=] the request |request|.
+              1. Let |preloadRequest| be a copy of |request|, except for its [=request/body=].

I don't see why we cannot clone here. Body will still have its source afterwards.

> @@ -3032,6 +3039,12 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
                   1. Wait until |e|'s [=FetchEvent/wait to respond flag=] is unset.
                   1. If |e|'s [=FetchEvent/respond-with error flag=] is set, set |handleFetchFailed| to true.
                   1. Else, set |response| to |e|'s [=FetchEvent/potential response=].
+              1. If |response| is null and |requestBody| is not null, then:
+                  1. If |requestBody|'s [=Body/source=] is null, then:
+                      1. If |requestBody|'s [=Body/stream=] is [=Body/disturbed=] or [=Body/locked=], then return a [=network error=] and null.

You want =Body/unusable= here.

> @@ -3032,6 +3039,12 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
                   1. Wait until |e|'s [=FetchEvent/wait to respond flag=] is unset.
                   1. If |e|'s [=FetchEvent/respond-with error flag=] is set, set |handleFetchFailed| to true.
                   1. Else, set |response| to |e|'s [=FetchEvent/potential response=].
+              1. If |response| is null and |requestBody| is not null, then:
+                  1. If |requestBody|'s [=Body/source=] is null, then:

This is unfortunate, but indeed something we're stuck with. Not sure if that's worth pointing out somehow.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1563#pullrequestreview-585632135