fix(service-worker): correctly determine client ID on navigation requ…

…ests

The ServiceWorker assigns an app-version to a each client to ensure that
all subsequent requests for a client are served using the same
app-version. The assignment is done based on the client ID.

Previously, the ServiceWorker would only try to read the client's ID off
of the `FetchEvent`'s `clientId` property. However, for navigation
requests the new client's ID will be set on [resultingClientId][1],
while `clientId` will either be empty or hold the ID of the client where
the request initiated from. See also related discussions in
w3c/ServiceWorker#870 and w3c/ServiceWorker#1266.

In theory, this could lead to the navigation request (i.e. `index.html`)
being served from a different app-version than the subsequent
sub-resource requests (i.e. assets). In practice, the likelihood of this
happening is probably very low though, since it would require the latest
app-version to be updated between the initial navigation request and the
first sub-resource request, which should happen very shortly after the
navigation request.

This commit ensures that the correct client ID is determined even for
navigation requests by also taking the `resultingClientId` property into
account.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/resultingClientId

packages/service-worker/worker/src/driver.ts

@@ -609,7 +609,8 @@ export class Driver implements Debuggable, UpdateSource {
   private async assignVersion(event: FetchEvent): Promise<AppVersion|null> {
     // First, check whether the event has a (non empty) client ID. If it does, the version may
     // already be associated.
-    const clientId = event.clientId;
+    // (For navigation requests, we care about the `resultingClientId`.)
+    const clientId = event.resultingClientId || event.clientId;
     if (clientId) {
       // Check if there is an assigned client id.
       if (this.clientVersionMap.has(clientId)) {

packages/service-worker/worker/src/service-worker.d.ts

@@ -60,8 +60,9 @@ type WindowClientState = 'hidden'|'visible'|'prerender'|'unloaded';
 // Fetch API
 
 interface FetchEvent extends ExtendableEvent {
-  clientId: string|null;
   request: Request;
+  clientId?: string;
+  resultingClientId?: string;
   respondWith(response: Promise<Response>|Response): Promise<Response>;
 }
 

packages/service-worker/worker/test/happy_spec.ts

@@ -538,7 +538,6 @@ describe('Driver', () => {
   it('handles empty client ID', async () => {
     // Initialize the SW.
     expect(await makeNavigationRequest(scope, '/foo/file1', '')).toEqual('this is foo');
-    expect(await makeNavigationRequest(scope, '/bar/file2', null)).toEqual('this is foo');
     await driver.initialized;
 
     // Update to a new version.
@@ -547,7 +546,6 @@ describe('Driver', () => {
 
     // Correctly handle navigation requests, even if `clientId` is null/empty.
     expect(await makeNavigationRequest(scope, '/foo/file1', '')).toEqual('this is foo v2');
-    expect(await makeNavigationRequest(scope, '/bar/file2', null)).toEqual('this is foo v2');
   });
 
   it('checks for updates on restart', async () => {
@@ -2008,8 +2006,7 @@ async function removeAssetFromCache(
 }
 
 async function makeRequest(
-    scope: SwTestHarness, url: string, clientId: string|null = 'default',
-    init?: Object): Promise<string|null> {
+    scope: SwTestHarness, url: string, clientId = 'default', init?: Object): Promise<string|null> {
   const [resPromise, done] = scope.handleFetch(new MockRequest(url, init), clientId);
   await done;
   const res = await resPromise;
@@ -2020,8 +2017,7 @@ async function makeRequest(
 }
 
 function makeNavigationRequest(
-    scope: SwTestHarness, url: string, clientId?: string|null,
-    init: Object = {}): Promise<string|null> {
+    scope: SwTestHarness, url: string, clientId?: string, init: Object = {}): Promise<string|null> {
   return makeRequest(scope, url, clientId, {
     headers: {
       Accept: 'text/plain, text/html, text/css',

packages/service-worker/worker/testing/scope.ts

@@ -219,12 +219,15 @@ export class SwTestHarness extends Adapter implements ServiceWorkerGlobalScope,
 
   waitUntil(promise: Promise<void>): void {}
 
-  handleFetch(req: Request, clientId: string|null = null):
+  handleFetch(req: Request, clientId = ''):
       [Promise<Response|undefined>, Promise<void>] {
     if (!this.eventHandlers.has('fetch')) {
       throw new Error('No fetch handler registered');
     }
-    const event = new MockFetchEvent(req, clientId);
+
+    const isNavigation = req.mode === 'navigate';
+    const event = isNavigation ?
+        new MockFetchEvent(req, '', clientId) : new MockFetchEvent(req, clientId, '');
     this.eventHandlers.get('fetch')!.call(this, event);
 
     if (clientId) {
@@ -373,7 +376,8 @@ class MockExtendableEvent extends OneTimeContext {}
 class MockFetchEvent extends MockExtendableEvent {
   response: Promise<Response|undefined> = Promise.resolve(undefined);
 
-  constructor(readonly request: Request, readonly clientId: string|null) {
+  constructor(
+      readonly request: Request, readonly clientId: string, readonly resultingClientId: string) {
     super();
   }