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

Add steps to intercept requests using foreign fetch. #329

Closed
wants to merge 4 commits into from

Conversation

mkruisselbrink
Copy link
Collaborator

If response is still null after a regular service worker has tried handling it
this gives a foreign fetch handler a chance to handle the request.

@annevk
Copy link
Member

annevk commented Jun 29, 2016

  1. This lacks a same-origin check that I think we want. The fetch needs to actually be foreign. I'm not sure if you're handling that on the other side, but I think it would make more sense here. Happy to discuss of course.
  2. A lot of this text is copy-and-pasted from the previous major step. If the requirements are actually identical, we should do the work to make that clear without duplication. I haven't quite figured out yet whether the "network error" requirements need to be the same, but CSP definitely makes sense.

@mkruisselbrink
Copy link
Collaborator Author

This lacks a same-origin check that I think we want. The fetch needs to actually be foreign. I'm not sure if you're handling that on the other side, but I think it would make more sense here. Happy to discuss of course.

Yeah, that check does exist on the service worker spec side as well, but I agree that it makes sense to have the check here as well. I also added the "is this a subresource" check here, to make it clearer which requests actually can be intercepted by foreign fetch.

A lot of this text is copy-and-pasted from the previous major step. If the requirements are actually identical, we should do the work to make that clear without duplication. I haven't quite figured out yet whether the "network error" requirements need to be the same, but CSP definitely makes sense.

Duplication is indeed unfortunate. Maybe one way to deal with this would be to move all the shared substeps to a separate step, so it roughly would be something like:

  1. If [....] invoke handle fetch.
  2. If response is null and [...], invoke handle foreign fetch.
  3. If response is not null:
    1. Set actualResponse
    2. Do the network error stuff
    3. Do the CSP stuff

@annevk
Copy link
Member

annevk commented Jun 30, 2016

Yeah, that check does exist on the service worker spec side as well, but I agree that it makes sense to have the check here as well.

I think in the end we should only have it on one side.

With regards to duplication, what you suggest seems reasonable. An alternative might be to group what you suggest under a common skip-service-worker flag check. Not entirely sure what works better.

@mkruisselbrink
Copy link
Collaborator Author

Okay, factored out the common post-processing and skip-service-worker flag logic (which has the side effect of also fixing that previously things like "set response's CSP list" were called even when response was null).

<li>
<p>If <var>response</var> is null,
<var>request</var> is a <span>subresource request</span> and <var>request</var>'s
<span title=concept-request-origin>origin</span> is not the same as <var>request</var>'s
Copy link
Member

Choose a reason for hiding this comment

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

"same" needs to reference "same origin" defined in HTML.

@annevk
Copy link
Member

annevk commented Jul 27, 2016

I like it!

If response is still null after a regular service worker has tried handling it
this gives a foreign fetch handler a chance to handle the request.
@mkruisselbrink
Copy link
Collaborator Author

Addressed your comments.

annevk pushed a commit that referenced this pull request Aug 4, 2016
If a same-origin service worker does not handle the request, then let
a cross-origin service worker handle it. This feature is called
Foreign Fetch and this commit adds the required Fetch infrastructure.

Fixes #223.

PR: #329
@annevk
Copy link
Member

annevk commented Aug 4, 2016

Landed as 8510b2f. Thanks for working on this!

@annevk annevk closed this Aug 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants