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

Meta: add a service worker to the spec #637

Merged
merged 8 commits into from
Jan 12, 2017
Merged

Conversation

domenic
Copy link
Member

@domenic domenic commented Dec 29, 2016

This allows the spec to be viewed offline.

/cc @jakearchibald as my service worker person. /cc @annevk since we may want to be deploying this to other specs.

The biggest open question to me is whether we want a cache-then-network strategy (done here), or network-then-cache strategy. The problem with a cache-then-network strategy is that it means that if the spec has changed since you last visited, you'll always see an outdated spec on your first visit. That seems kind of bad for a living standard. Hmm, maybe I will switch it in a second commit.

I might also push a commit adding a web app manifest and stuff.

@domenic
Copy link
Member Author

domenic commented Dec 29, 2016

I switched to a network-then-cache strategy in the latest commit. Although, playing around with Chrome dev tools and throttling to slow connections, it's pretty painful; it would be kind of nice if we could get instant loading for those cases. A tricky dillema.

@domenic domenic force-pushed the serviceworker-the-spec branch from 0061e69 to 45df571 Compare December 29, 2016 19:53
@domenic
Copy link
Member Author

domenic commented Dec 29, 2016

Added a manifest, although its benefits are limited as Chrome does not support splash screens or auto-add-to-homescreen prompts for "display": "browser" pages, and I don't feel comfortable hiding the URL bar for a spec.

# Living standard, if master
curl https://api.csswg.org/bikeshed/ -f -F [email protected] \
-F md-Text-Macro="SNAPSHOT-LINK $SNAPSHOT_LINK" \
> $WEB_ROOT/index.intermediate.html
node_modules/.bin/emu-algify --throwing-indicators < $WEB_ROOT/index.intermediate.html > $WEB_ROOT/index.html
rm $WEB_ROOT/index.intermediate.html

cp service-worker.js $WEB_ROOT/service-worker.js
cp manifest.json $WEB_ROOT/manifest.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the standard file extension .webmanifest these days? w3c/manifest#381

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. I can't see the benefit of using a different file extension. In the end I might drop this commit though, as the manifest gives almost no benefit it seems.


e.respondWith(
// Since this is a Living Standard, it is imperative that you see the freshest content, so we use a
// network-then-cache strategy.
Copy link
Member

Choose a reason for hiding this comment

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

Should we perhaps only do this for the main content, not the subresources?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great call, the spec is much faster now when throttled even to 2G.

@@ -60,6 +60,7 @@ urlPrefix: https://tc39.github.io/ecma262/; spec: ECMASCRIPT
</style>
<script src="https://resources.whatwg.org/file-issue.js" async></script>
<script src="https://resources.whatwg.org/commit-snapshot-shortcut-key.js" async></script>
<link rel="manifest" href="/manifest.json">
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little skeptical of the value-add here. It's only used for "installation" purposes reportedly and all the metadata included is already readily available through other means in the document.

@domenic
Copy link
Member Author

domenic commented Dec 30, 2016

Regarding the manifest, pulling @annevk's comments out to top level on the thread so they don't get lost when I revert:

I'm a little skeptical of the value-add here. It's only used for "installation" purposes reportedly and all the metadata included is already readily available through other means in the document.

Yeah. I think I'll remove it. Given that Chrome refuses to give us a splash screen or an install-to-homescreen prompt with display: browser, the only benefits in the current browser landscape are that, if you manually choose Add to Homescreen from the Chrome mobile overflow menu, it will use the shortname "Streams" as the default name, and if you happen to try adding a commit snapshot to the homescreen, it will instead change the default launch URL to /.

Maybe manifests are worth revisiting if Chrome becomes less hostile to display: browser, or if Firefox or other browsers finish their implementation.


self.onactivate = e => {
e.waitUntil(caches.keys().then(keys => {
return Promise.all(keys.filter(key => key !== cacheKey).map(key => caches.delete(key)));

Choose a reason for hiding this comment

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

I'm guessing this will be the only service worker on this origin? A more general solution might not be able to assume this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep :)


const resForCache = res.clone();

// Do not return this promise; it's OK if caching fails, and we don't want to block on it.

Choose a reason for hiding this comment

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

You could pass it to fetchEvent.waitUntil - it doesn't delay the return of the response to the browser, but it ensures the SW is kept awake until caching is complete.

We updated the spec so waitUntil could be called async, but it hasn't landed in browsers yet. Polyfill: https://github.com/jakearchibald/async-waituntil-polyfill

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh fascinating. Will things break if we just pass it to fetchEvent.waitUntil for now, and let it just not work until browsers update themselves?

@jakearchibald
Copy link

jakearchibald commented Jan 4, 2017

As always, the trickiest bit of offline-first is the UI. Some thoughts for how it could work:

  1. Race the network & cache for a short amount of time (1 second?).
  2. If the network wins - excellent! Serve it. Done!
  3. if the cache wins, include UI that shows the date the content was last fetched, and a spinner while it's fetching.
  4. If fetching fails, hide the spinner and notify the user.
  5. If fetching succeeds:
    1. If the document hasn't been updated, hide the spinner, notify the user. Done!
    2. If the document has been updated, notify the user and give them a button to refresh the page.

@domenic
Copy link
Member Author

domenic commented Jan 4, 2017

Yeah, I contemplated more tricky cache-first solutions involving postMessaging back and forth to the main thread. I think they might be good as follow-ups, but for now the cache-first-for-subresources, network-first-for-main-document seems like a pretty good compromise.

@domenic domenic force-pushed the serviceworker-the-spec branch from 741d344 to f2919bd Compare January 12, 2017 22:10
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.

4 participants