Skip to content

I
interface
Commit dd5feaac authored by Zach Pomerantz's avatar Zach Pomerantz Committed by GitHub
Browse files
Options

fix: serviceworker request path (#3926) 

* fix: serviceworker request path

Always requests the app-shell from the same path as the cache key, in
order to guarantee that the etags will match should the cache be valid.

* fix: avoid returning redirects
parent cc919ab3
Show whitespace changes
Inline Side-by-side
Showing with 46 additions and 49 deletions
src/serviceWorker/document.test.ts
View file @ dd5feaac
import { RouteHandlerCallbackOptions, RouteMatchCallbackOptions } from 'workbox-core' import { RouteHandlerCallbackOptions, RouteMatchCallbackOptions } from 'workbox-core'
import { matchPrecache as matchPrecacheMock } from 'workbox-precaching' import { getCacheKeyForURL as getCacheKeyForURLMock, matchPrecache as matchPrecacheMock } from 'workbox-precaching'
import { CachedDocument, DOCUMENT, handleDocument, matchDocument } from './document' import { CachedDocument, handleDocument, matchDocument } from './document'
jest.mock('workbox-navigation-preload', () => ({ enable: jest.fn() })) jest.mock('workbox-navigation-preload', () => ({ enable: jest.fn() }))
jest.mock('workbox-precaching', () => ({ matchPrecache: jest.fn() })) jest.mock('workbox-precaching', () => ({
getCacheKeyForURL: jest.fn(),
matchPrecache: jest.fn(),
}))
jest.mock('workbox-routing', () => ({ Route: class {} })) jest.mock('workbox-routing', () => ({ Route: class {} }))
describe('document', () => { describe('document', () => {
...@@ -29,17 +32,22 @@ describe('document', () => { ...@@ -29,17 +32,22 @@ describe('document', () => {
}) })
describe('handleDocument', () => { describe('handleDocument', () => {
const requestUrl = 'request_url'
let fetch: jest.SpyInstance let fetch: jest.SpyInstance
let getCacheKeyForURL: jest.SpyInstance
let matchPrecache: jest.SpyInstance let matchPrecache: jest.SpyInstance
let options: RouteHandlerCallbackOptions let options: RouteHandlerCallbackOptions
beforeAll(() => { beforeAll(() => {
fetch = jest.spyOn(window, 'fetch') fetch = jest.spyOn(window, 'fetch')
getCacheKeyForURL = getCacheKeyForURLMock as unknown as jest.SpyInstance
matchPrecache = matchPrecacheMock as unknown as jest.SpyInstance matchPrecache = matchPrecacheMock as unknown as jest.SpyInstance
}) })
beforeEach(() => { beforeEach(() => {
fetch.mockReset() fetch.mockReset()
getCacheKeyForURL.mockReturnValueOnce(requestUrl)
options = { options = {
event: new Event('fetch') as ExtendableEvent, event: new Event('fetch') as ExtendableEvent,
request: new Request('http://example.com'), request: new Request('http://example.com'),
...@@ -57,11 +65,11 @@ describe('document', () => { ...@@ -57,11 +65,11 @@ describe('document', () => {
afterAll(() => onLine.mockRestore()) afterAll(() => onLine.mockRestore())
it('returns a fetched document', async () => { it('returns a fetched document', async () => {
const fetched = new Response() const fetched = new Response('test_body')
fetch.mockResolvedValueOnce(fetched) fetch.mockResolvedValueOnce(fetched)
const response = await handleDocument(options) const response = await handleDocument(options)
expect(fetch).toHaveBeenCalledWith(options.request) expect(fetch).toHaveBeenCalledWith(options.request)
expect(response).toBe(fetched) expect(response.body).toBe(fetched.body)
}) })
it('returns a clone of offlineDocument with an offlineDocument', async () => { it('returns a clone of offlineDocument with an offlineDocument', async () => {
...@@ -76,11 +84,12 @@ describe('document', () => { ...@@ -76,11 +84,12 @@ describe('document', () => {
describe('with a thrown fetch', () => { describe('with a thrown fetch', () => {
it('returns a cached response', async () => { it('returns a cached response', async () => {
const cached = new Response() const cached = new Response('<html><head></head></html>')
matchPrecache.mockResolvedValueOnce(cached) matchPrecache.mockResolvedValueOnce(cached)
fetch.mockRejectedValueOnce(new Error()) fetch.mockRejectedValueOnce(new Error())
const { response } = (await handleDocument(options)) as CachedDocument const response = await handleDocument(options)
expect(response).toBe(cached) expect(response).toBeInstanceOf(CachedDocument)
expect((response as CachedDocument).response).toBe(cached)
}) })
it('rethrows with no cached response', async () => { it('rethrows with no cached response', async () => {
...@@ -90,35 +99,24 @@ describe('document', () => { ...@@ -90,35 +99,24 @@ describe('document', () => {
}) })
}) })
describe.each([ describe('with a fetched response', () => {
['preloadResponse', true],
['fetched document', false],
])('with a %s', (responseType, withPreloadResponse) => {
let fetched: Response let fetched: Response
const FETCHED_ETAGS = 'fetched' const FETCHED_ETAGS = 'fetched'
beforeEach(() => { beforeEach(() => {
fetched = new Response(null, { headers: { etag: FETCHED_ETAGS } }) fetched = new Response('test_body', { headers: { etag: FETCHED_ETAGS } })
if (withPreloadResponse) {
;(options.event as { preloadResponse?: Promise<Response> }).preloadResponse = Promise.resolve(fetched)
} else {
fetch.mockReturnValueOnce(fetched) fetch.mockReturnValueOnce(fetched)
}
}) })
afterEach(() => { afterEach(() => {
if (withPreloadResponse) { expect(fetch).toHaveBeenCalledWith(requestUrl, expect.anything())
expect(fetch).not.toHaveBeenCalled()
} else {
expect(fetch).toHaveBeenCalledWith(DOCUMENT, expect.anything())
}
}) })
describe('with a cached response', () => { describe('with a cached response', () => {
let cached: Response let cached: Response
beforeEach(() => { beforeEach(() => {
cached = new Response('<html>cached</html>', { headers: { etag: 'cached' } }) cached = new Response('<html><head></head></html>', { headers: { etag: 'cached' } })
matchPrecache.mockResolvedValueOnce(cached) matchPrecache.mockResolvedValueOnce(cached)
}) })
...@@ -127,29 +125,31 @@ describe('document', () => { ...@@ -127,29 +125,31 @@ describe('document', () => {
cached.headers.set('etag', FETCHED_ETAGS) cached.headers.set('etag', FETCHED_ETAGS)
}) })
if (!withPreloadResponse) {
it('aborts the fetched response', async () => { it('aborts the fetched response', async () => {
await handleDocument(options) await handleDocument(options)
const abortSignal = fetch.mock.calls[0][1].signal const abortSignal = fetch.mock.calls[0][1].signal
expect(abortSignal.aborted).toBeTruthy() expect(abortSignal.aborted).toBeTruthy()
}) })
}
it('returns the cached response', async () => { it('returns the cached response', async () => {
const { response } = (await handleDocument(options)) as CachedDocument const response = await handleDocument(options)
expect(response).toBe(cached) expect(response).toBeInstanceOf(CachedDocument)
expect((response as CachedDocument).response).toBe(cached)
expect(await response.text()).toBe(
'<html><head><script>window.__isDocumentCached=true</script></head></html>'
)
}) })
}) })
it(`returns the ${responseType} with mismatched etags`, async () => { it(`returns the fetched response with mismatched etags`, async () => {
const response = await handleDocument(options) const response = await handleDocument(options)
expect(response).toBe(fetched) expect(response.body).toBe(fetched.body)
}) })
}) })
it(`returns the ${responseType} with no cached response`, async () => { it(`returns the fetched response with no cached response`, async () => {
const response = await handleDocument(options) const response = await handleDocument(options)
expect(response).toBe(fetched) expect(response.body).toBe(fetched.body)
}) })
}) })
}) })
......
src/serviceWorker/document.ts
View file @ dd5feaac
import { RouteHandlerCallbackOptions, RouteMatchCallbackOptions } from 'workbox-core' import { RouteHandlerCallbackOptions, RouteMatchCallbackOptions } from 'workbox-core'
import * as navigationPreload from 'workbox-navigation-preload' import { getCacheKeyForURL, matchPrecache } from 'workbox-precaching'
import { matchPrecache } from 'workbox-precaching'
import { Route } from 'workbox-routing' import { Route } from 'workbox-routing'
import { isLocalhost } from './utils' import { isLocalhost } from './utils'
...@@ -39,7 +38,7 @@ type HandlerContext = { ...@@ -39,7 +38,7 @@ type HandlerContext = {
/** /**
* The returned document should always be fresh, so this handler uses a custom strategy: * The returned document should always be fresh, so this handler uses a custom strategy:
* *
* - Always fetches the document (using navigationPreload, if supported). * - Always fetches the document.
* - When available, compares the etag headers of the fetched and cached documents: * - When available, compares the etag headers of the fetched and cached documents:
* - If matching (fresh) or missing (offline), returns the cached document. * - If matching (fresh) or missing (offline), returns the cached document.
* - If not matching (stale), returns the fetched document. * - If not matching (stale), returns the fetched document.
...@@ -54,17 +53,18 @@ export async function handleDocument(this: HandlerContext, { event, request }: R ...@@ -54,17 +53,18 @@ export async function handleDocument(this: HandlerContext, { event, request }: R
// If we are offline, serve the offline document. // If we are offline, serve the offline document.
if ('onLine' in navigator && !navigator.onLine) return this?.offlineDocument?.clone() || fetch(request) if ('onLine' in navigator && !navigator.onLine) return this?.offlineDocument?.clone() || fetch(request)
// Always use index.html, as its already been matched for App Shell-style routing (@see {@link matchDocument}). // The exact cache key should be used for requests, as etags will be different for different paths.
// This also prevents usage of preloadResponse.
const requestUrl = getCacheKeyForURL(DOCUMENT)
const cachedResponse = await matchPrecache(DOCUMENT) const cachedResponse = await matchPrecache(DOCUMENT)
const { preloadResponse } = event as unknown as { preloadResponse: Promise<Response | undefined> }
// Responses will throw if offline, but if cached the cached response should still be returned. // Responses will throw if offline, but if cached the cached response should still be returned.
const controller = new AbortController() const controller = new AbortController()
let response let response
try { try {
response = (await preloadResponse) || (await fetch(DOCUMENT, { signal: controller.signal })) response = await fetch(requestUrl || DOCUMENT, { cache: 'reload', signal: controller.signal })
if (!cachedResponse) { if (!cachedResponse) {
return response return new Response(response.body, response)
} }
} catch (e) { } catch (e) {
if (!cachedResponse) throw e if (!cachedResponse) throw e
...@@ -76,19 +76,16 @@ export async function handleDocument(this: HandlerContext, { event, request }: R ...@@ -76,19 +76,16 @@ export async function handleDocument(this: HandlerContext, { event, request }: R
const etag = response?.headers.get('etag') const etag = response?.headers.get('etag')
const cachedEtag = cachedResponse?.headers.get('etag') const cachedEtag = cachedResponse?.headers.get('etag')
if (etag && etag === cachedEtag) { if (etag && etag === cachedEtag) {
// If the cache is still fresh, cancel the pending response. The preloadResponse is cancelled // If the cache is still fresh, cancel the pending response.
// automatically by returning before it is settled; cancelling the preloadResponse will log
// an error to the console, but it can be ignored - it *should* be cancelled.
controller.abort() controller.abort()
return CachedDocument.from(cachedResponse) return CachedDocument.from(cachedResponse)
} }
return response return new Response(response.body, response)
} }
export class DocumentRoute extends Route { export class DocumentRoute extends Route {
constructor(offlineDocument?: Response) { constructor(offlineDocument?: Response) {
navigationPreload.enable()
super(matchDocument, handleDocument.bind({ offlineDocument }), 'GET') super(matchDocument, handleDocument.bind({ offlineDocument }), 'GET')
} }
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment