Re: [w3ctag/design-reviews] Navigation Preload for Service Worker (#166)

This feedback is very first-impression and a lot of it likely is just cause to consider including more reasonsing in the docs so that the purpose of the design is clearer.  To be discussed with @slightlyoff later today.

## Activating is async?

It seems weird to wait on a promise in `activate` when, presumably confirming that preload got enabled is not a dependency of anything else in activate?  I'm also confused by why this is async at all, since it doesn't touch network or document, so aren't it's performance characteristics a) well known and predictable, and b) very fast?

I thought Alex said earlier that `enable` can take args, but I can't see that [in the spec](https://github.com/w3c/ServiceWorker/pull/983/files#diff-27b79860afe28f01aed4f1f6228367faR766)

## Checking for a preload response

This was a bit hard to reason about for me:

```
const response = await event.preloadResponse;
if (response) return response;
```

The key point is that if there is *not going to be* a preload response, it will resolve immediately with undefined.  If there is no preload response yet, but there's a preload request in flight, it will wait on that and resolve with the response...

I would have imagined that the existence of a promise would indicate that there's something to wait for:

```
if (event.preloadResponsePromise) {
  const response = await event.preloadResponsePromise;
  return response;
}
```

Later Jake notes that using `Promise.resolve` wrapper around `event.preloadResponse` is a good way to normalise browsers that don't support preload, but the above method would avoid that problem.

As an aside it's kind of annoying that standard web platform features that return promises are not named to indicate that, so you just have to know that `preloadResponse` is not a response but a *promise of a response*.

## Merging streams

This is not NavigationPreload related, but the explainer post includes an example of using a static header/footer.  Jake keeps using this concept in an 'article' context and I'm confused by this because I can't think of any publishing use cases in which a page header would be sufficiently static to use this technique.  Do you know of anyone doing it?

## Header

The example of using the header was confusing me.  I think a sequence of steps would help:

1. Page requests `/article/123`
2. SW hasn't booted yet, so we send a preload request to server for that URL
3. Server sees that this is a preload, so despite the fact that the URL does not end in `.include`, it returns content as if it did.
3. When the SW boots, the fetch event handler is called for the `/article/123` request, and there's a preloadResponse available.
4. We use the preloadResponse as an include and sandwich it in the static header and footer.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3ctag/design-reviews/issues/166#issuecomment-297611731