Skip to content

Remove foreign fetch #1207

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
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

jakearchibald commented Oct 13, 2017

Creating a PR to remove the tests…

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

jakearchibald commented Oct 13, 2017

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
</tr>
</thead>
<tbody>
<tr>
Copy link
Collaborator

@mkruisselbrink mkruisselbrink Oct 13, 2017

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

@jakearchibald jakearchibald Oct 13, 2017

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

jakearchibald commented Oct 13, 2017

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

jakearchibald commented Oct 25, 2017

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

wanderview commented Oct 25, 2017

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

jakearchibald commented Nov 7, 2017

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

jungkees commented Nov 16, 2017

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

jakearchibald commented Nov 16, 2017

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

jakearchibald commented Nov 16, 2017

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
@sisidovski sisidovski mentioned this pull request Sep 27, 2024
Merged
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.

5 participants

@jakearchibald @wanderview @jungkees @mkruisselbrink