From efa58efd0c7f9a157abce879a550d8bba40d8b40 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Tue, 27 Apr 2021 02:49:28 -0500 Subject: [PATCH] Ensure stalled CSS triggers fallback navigation (#24488) This ensures when CSS requests stall that they are included in the route load timeout so that stalled CSS requests don't block us from falling back to a hard navigation. This handles a rare case noticed by @pacocoursey where a transition did not complete while attempting to load CSS assets. ## Bug - [ ] Related issues linked using `fixes #number` - [x] Integration tests added --- packages/next/client/route-loader.ts | 66 +++++++++++-------- .../build-output/test/index.test.js | 4 +- .../css-client-nav/test/index.test.js | 63 +++++++++++++++++- 3 files changed, 101 insertions(+), 32 deletions(-) diff --git a/packages/next/client/route-loader.ts b/packages/next/client/route-loader.ts index b6157dd2efcd3..dbe9e842e272e 100644 --- a/packages/next/client/route-loader.ts +++ b/packages/next/client/route-loader.ts @@ -68,7 +68,7 @@ function withFuture( export interface RouteLoader { whenEntrypoint(route: string): Promise onEntrypoint(route: string, execute: () => unknown): void - loadRoute(route: string): Promise + loadRoute(route: string, prefetch?: boolean): Promise prefetch(route: string): Promise } @@ -305,33 +305,41 @@ function createRouteLoader(assetPrefix: string): RouteLoader { if (old && 'resolve' in old) old.resolve(input) }) }, - loadRoute(route: string) { - return withFuture(route, routes, async () => { - try { - const { scripts, css } = await getFilesForRoute(assetPrefix, route) - const [, styles] = await Promise.all([ - entrypoints.has(route) - ? [] - : Promise.all(scripts.map(maybeExecuteScript)), - Promise.all(css.map(fetchStyleSheet)), - ] as const) - - const entrypoint: RouteEntrypoint = await resolvePromiseWithTimeout( - this.whenEntrypoint(route), - MS_MAX_IDLE_DELAY, - markAssetError( - new Error(`Route did not complete loading: ${route}`) - ) - ) - - const res: RouteLoaderEntry = Object.assign< - { styles: RouteStyleSheet[] }, - RouteEntrypoint - >({ styles }, entrypoint) - return 'error' in entrypoint ? entrypoint : res - } catch (err) { - return { error: err } - } + loadRoute(route: string, prefetch?: boolean) { + return withFuture(route, routes, () => { + return resolvePromiseWithTimeout( + getFilesForRoute(assetPrefix, route) + .then(({ scripts, css }) => { + return Promise.all([ + entrypoints.has(route) + ? [] + : Promise.all(scripts.map(maybeExecuteScript)), + Promise.all(css.map(fetchStyleSheet)), + ] as const) + }) + .then((res) => { + return this.whenEntrypoint(route).then((entrypoint) => ({ + entrypoint, + styles: res[1], + })) + }), + MS_MAX_IDLE_DELAY, + markAssetError(new Error(`Route did not complete loading: ${route}`)) + ) + .then(({ entrypoint, styles }) => { + const res: RouteLoaderEntry = Object.assign< + { styles: RouteStyleSheet[] }, + RouteEntrypoint + >({ styles: styles! }, entrypoint) + return 'error' in entrypoint ? entrypoint : res + }) + .catch((err) => { + if (prefetch) { + // we don't want to cache errors during prefetch + throw err + } + return { error: err } + }) }) }, prefetch(route: string): Promise { @@ -351,7 +359,7 @@ function createRouteLoader(assetPrefix: string): RouteLoader { ) ) .then(() => { - requestIdleCallback(() => this.loadRoute(route)) + requestIdleCallback(() => this.loadRoute(route, true).catch(() => {})) }) .catch( // swallow prefetch errors diff --git a/test/integration/build-output/test/index.test.js b/test/integration/build-output/test/index.test.js index 6d216d5a6b576..1913adfe429f0 100644 --- a/test/integration/build-output/test/index.test.js +++ b/test/integration/build-output/test/index.test.js @@ -99,7 +99,7 @@ describe('Build Output', () => { expect(indexSize.endsWith('B')).toBe(true) // should be no bigger than 64.8 kb - expect(parseFloat(indexFirstLoad)).toBeCloseTo(65.4, 1) + expect(parseFloat(indexFirstLoad)).toBeCloseTo(65.3, 1) expect(indexFirstLoad.endsWith('kB')).toBe(true) expect(parseFloat(err404Size)).toBeCloseTo(3.69, 1) @@ -108,7 +108,7 @@ describe('Build Output', () => { expect(parseFloat(err404FirstLoad)).toBeCloseTo(68.8, 0) expect(err404FirstLoad.endsWith('kB')).toBe(true) - expect(parseFloat(sharedByAll)).toBeCloseTo(65.1, 1) + expect(parseFloat(sharedByAll)).toBeCloseTo(65, 1) expect(sharedByAll.endsWith('kB')).toBe(true) if (_appSize.endsWith('kB')) { diff --git a/test/integration/css-client-nav/test/index.test.js b/test/integration/css-client-nav/test/index.test.js index 2a3d2eea03ce9..986e60d5f01c9 100644 --- a/test/integration/css-client-nav/test/index.test.js +++ b/test/integration/css-client-nav/test/index.test.js @@ -1,5 +1,7 @@ /* eslint-env jest */ +import http from 'http' +import httpProxy from 'http-proxy' import cheerio from 'cheerio' import { remove } from 'fs-extra' import { @@ -18,6 +20,8 @@ jest.setTimeout(1000 * 60 * 1) const fixturesDir = join(__dirname, '../../css-fixtures') const appDir = join(fixturesDir, 'multi-module') +let proxyServer +let stallCss let appPort let app @@ -152,12 +156,69 @@ describe('CSS Module client-side navigation', () => { beforeAll(async () => { await remove(join(appDir, '.next')) await nextBuild(appDir) + const port = await findPort() + app = await nextStart(appDir, port) appPort = await findPort() - app = await nextStart(appDir, appPort) + + const proxy = httpProxy.createProxyServer({ + target: `http://localhost:${port}`, + }) + + proxyServer = http.createServer(async (req, res) => { + if (stallCss && req.url.endsWith('.css')) { + console.log('stalling request for', req.url) + await new Promise((resolve) => setTimeout(resolve, 5 * 1000)) + } + proxy.web(req, res) + }) + + proxy.on('error', (err) => { + console.warn('Failed to proxy', err) + }) + + await new Promise((resolve) => { + proxyServer.listen(appPort, () => resolve()) + }) }) afterAll(async () => { + proxyServer.close() await killApp(app) }) + + it('should time out and hard navigate for stalled CSS request', async () => { + let browser + stallCss = true + + try { + browser = await webdriver(appPort, '/red') + browser.eval('window.beforeNav = "hello"') + + const redColor = await browser.eval( + `window.getComputedStyle(document.querySelector('#verify-red')).color` + ) + expect(redColor).toMatchInlineSnapshot(`"rgb(255, 0, 0)"`) + expect(await browser.eval('window.beforeNav')).toBe('hello') + + await browser.elementByCss('#link-blue').click() + + await browser.waitForElementByCss('#verify-blue') + + const blueColor = await browser.eval( + `window.getComputedStyle(document.querySelector('#verify-blue')).color` + ) + expect(blueColor).toMatchInlineSnapshot(`"rgb(0, 0, 255)"`) + + // the timeout should have been reached and we did a hard + // navigation + expect(await browser.eval('window.beforeNav')).toBe(null) + } finally { + stallCss = false + if (browser) { + await browser.close() + } + } + }) + runTests() })