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

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.
4. When the SW boots, the fetch event handler is called for the /article/123 request, and there's a preloadResponse available.
5. We use the preloadResponse as an include and sandwich it in the static header and footer.

@triblondon

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?

It isn't essential, but it guarantees it's enabled for your first fetch event. Otherwise, if your code assumes that fetchEvent.preloadResponsewill give you a response for, say, navigation requests, you've got a race condition.

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?

Async is generally used where the source/destination of the information is potentially in another thread. In this case, the worker thread and underlying service worker registration are in different threads, they involve I/O, and can be accessed from multiple contexts.

Another example of this is indexeddb - the operations aren't to do with network, but everything is async.

I thought Alex said earlier that enable can take args

It can't. He might be thinking of setHeaderValue.

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

That was my original design, but it's (I'm told) against the rules of promises. With promises, you shouldn't mix sync and async, it's one or the other, so attribute promises should never be undefined, they should resolve with undefined.

This has come up a lot though.

Later Jake notes that using Promise.resolve wrapper around event.preloadResponse is a good way to normalise browsers that don't support preload

This isn't a problem with async functions, where await implicitly uses Promise.resolve.

it's kind of annoying that standard web platform features that return promises are not named to indicate that

I think in general, most properties don't reference their type. Eg document.body isn't document.bodyHTMLBodyElement, document.title isn't document.titleString etc etc.

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?

Most native apps follow this pattern. A static shell is delivered without connectivity, and the middle is filled in with network data.

In terms of on-the-web using streams specifically, it's what Facebook are looking to do, but they're using it to inject JSON into a static template. FWIW I'm merging streams on my blog https://jakearchibald.com/.

@jakearchibald I've fallen for the problem of publicly posting a load of note-to-self half baked thoughts from a first pass at understanding something someone has spent a lot of time drafting, and prompting a response from the person who cares about it deeply. I'm sorry if it seemed a bit ignorant.

We've now had the TAG discussion, and I better understand the design decisions. They make sense.

As a curiosity, the types thing in my mind is not quite the same as labelling all types, because promises to some extent obscure a 'real' type, which could be anything. Having said that it seems like you can await something that is not a promise and it will just pass it through, so I'm wondering why your article needs to do Promise.resolve(event.preloadResponse) to normalise. Is that because in that example you're using promise syntax rather than await?

We discussed the possibility of multiple headers, eg navigationPreload.setHeaders({...}), but clearly not necessary for the initial impl, as a developer could put a serialisation of something more complex into the single header if they want to.

Regarding the streaming header pattern, sending something that doesn't extend further than </head> or does a bit of lightweight templating in the SW probably resolves my concern over the utility of that in more complex cases.

In summary, 👍. We know you have a choice of review bodies for your peer review needs. We thank you for choosing TAG for your spec review today, please come again.