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.

Sign up for GitHub

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

Jump to bottom

Remove foreign fetch #1207

Merged
merged 3 commits into from
Nov 16, 2017
Merged

Remove foreign fetch #1207

merged 3 commits into from
Nov 16, 2017

Conversation

jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Oct 13, 2017 •
edited by pr-preview bot
Loading

Removing the foreign fetch parts. I've left the stuff in there for now, we may discuss that at TPAC.

Preview | Diff

@jakearchibald
Copy link
Contributor Author

Creating a PR to remove the tests…

@jakearchibald jakearchibald mentioned this pull request Oct 13, 2017
Merged
@jakearchibald
Copy link
Contributor Author

web-platform-tests/wpt#7762

mkruisselbrink
mkruisselbrink reviewed Oct 13, 2017
View reviewed changes
Copy link
Collaborator

@mkruisselbrink mkruisselbrink left a comment

Choose a reason for hiding this comment

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

.I've left the stuff in there for now

"The stuff"? Do you mean the link header stuff? Yeah, discussing at TPAC makes sense for that.

docs/index.bs Outdated
@@ -1725,11 +1557,6 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
</thead>
<tbody>
<tr>
<td><dfn event id="service-worker-global-scope-install-event"><code>install</code></dfn></td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This block should still be there, right? Just with ExtendableEvent rather than InstallEvent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, correct

@jakearchibald
Copy link
Contributor Author

Hah, I forgot to escape the html, I meant the <link rel=serviceworker> stuff

jungkees
jungkees approved these changes Oct 16, 2017
View reviewed changes
Copy link
Collaborator

@jungkees jungkees left a comment

Choose a reason for hiding this comment

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

I defer to @mkruisselbrink's review for this.

@jakearchibald
Copy link
Contributor Author

Before I land this… should we keep InstallEvent now we have it? Or just reintroduce it later when we need it?

@wanderview
Copy link
Member

Before I land this… should we keep InstallEvent now we have it? Or just reintroduce it later when we need it?

Currently we have the weird thing in waitUntil() steps that does special logic for install:

https://w3c.github.io/ServiceWorker/#wait-until-method

If we kept InstallEvent we could move that to an InstallEvent.waitUntil() that implements that behavior instead of special casing it ExtendableEvent. Not sure if thats worth it or not.

@jakearchibald jakearchibald mentioned this pull request Nov 3, 2017
Open
@jakearchibald
Copy link
Contributor Author

F2F: Keeping the header (for navigation requests) is interesting to CDNs. We'll talk to them this week and find out how important it is.

Going back to ExtendableEvent for install seems fine. Mozilla still uses ExtendableEvent.

@jungkees
Copy link
Collaborator

If we kept InstallEvent we could move that to an InstallEvent.waitUntil() that implements that behavior instead of special casing it ExtendableEvent. Not sure if thats worth it or not.

That behavior's already spec'ed in Install algorithm and Activate algorithm. I think we can safely change the prose in the waitUntil section to notes. Filed #1227.

So, we can just use ExtendableEvent as discussed in the F2F.

@jakearchibald jakearchibald merged commit 0d09f48 into master Nov 16, 2017
@jakearchibald
Copy link
Contributor Author

I've removed the link header & tag, but we should talk more to CDNs and add it again if needed.

Still TODO: remove from tests and HTML spec.

@jakearchibald
Copy link
Contributor Author

HTML PR: whatwg/html#3233
Tests PR: web-platform-tests/wpt#7762

@jungkees jungkees mentioned this pull request Nov 17, 2017
Closed
@annevk annevk deleted the remove-foreign-fetch branch November 22, 2017 14:14
@annevk annevk mentioned this pull request Nov 22, 2017
Merged
jakearchibald added a commit to web-platform-tests/wpt that referenced this pull request Nov 22, 2017
@jakearchibald
Removing foreign fetch tests (#7762)
1433896 
Reaction to:
whatwg/html#3233
w3c/ServiceWorker#1207
annevk pushed a commit to whatwg/html that referenced this pull request Nov 22, 2017
@jakearchibald @annevk
Remove <link rel=serviceworker> for now
52d09c9 
This is being removed as part of foreign fetch, but we may add it again once we have a better understanding of the use cases.

Tests removed in: web-platform-tests/wpt#7762.

More context: w3c/ServiceWorker#1207.
@rektide rektide mentioned this pull request Mar 28, 2018
Closed
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
@jakearchibald
Remove <link rel=serviceworker> for now
d9eecce 
This is being removed as part of foreign fetch, but we may add it again once we have a better understanding of the use cases.

Tests removed in: web-platform-tests/wpt#7762.

More context: w3c/ServiceWorker#1207.
@saschanaz saschanaz mentioned this pull request Mar 10, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkruisselbrink mkruisselbrink mkruisselbrink left review comments

@jungkees jungkees jungkees approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jakearchibald @wanderview @jungkees @mkruisselbrink