Skip to content
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

Test cases where request body gets used before network fallback #27325

Merged
merged 8 commits into from
Feb 18, 2021

Conversation

yutakahirano
Copy link
Contributor

Tests for whatwg/fetch#1144.

@yutakahirano
Copy link
Contributor Author

@annevk @yoichio PATL

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

I think we should also test the service worker cloning the request or the stream? Those seem like the kind of cases that might get overlooked in a naïve implementation.

service-workers/service-worker/fetch-event.https.html Outdated Show resolved Hide resolved
@@ -155,6 +155,12 @@ function handleIsHistoryNavigation(event) {
event.respondWith(new Response(body));
}

function handleUseAndIgnore(event) {
const request = event.request;
request.text();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should assert the body value here as well or in a different test. Otherwise it's still unclear whether the server worker can read it.

Copy link
Contributor Author

@yutakahirano yutakahirano Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is tested elsewhere: see "FetchEvent#body is a ReadableStream" in fetch-event.https.html.

@yutakahirano
Copy link
Contributor Author

I think we should also test the service worker cloning the request or the stream? Those seem like the kind of cases that might get overlooked in a naïve implementation.

I added "clone-and-ignore" tests.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good from my side modulo that nit. I'd like a service worker peer to also review this, e.g., @wanderview, @jakearchibald, or @asutherland.

@yutakahirano
Copy link
Contributor Author

Added tests for whatwg/fetch#572.

annevk pushed a commit to whatwg/fetch that referenced this pull request Feb 18, 2021
Also stop transmitting request's body in this scenario.

Service worker PR: w3c/ServiceWorker#1563.

Tests: web-platform-tests/wpt#27325.

Fixes w3c/ServiceWorker#1560 and fixes #572.
@annevk annevk requested a review from jakearchibald February 18, 2021 14:34
Copy link
Contributor

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (I wasn't aware that going via a SW breaks XHR upload events, but I understand why)

@yutakahirano yutakahirano merged commit c30545d into master Feb 18, 2021
@yutakahirano yutakahirano deleted the yhirano/streaming-upload-sw-passthrough branch February 18, 2021 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants