From 2795b79959954a4c1a2dddec2086f74ee3f6bf6f Mon Sep 17 00:00:00 2001 From: Gerald Monaco Date: Tue, 6 Jul 2021 02:41:51 -0700 Subject: [PATCH] Add initial `ResponsePayload` support (#26938) Refactors the internals of `next-server` to use `ResponsePayload` instead of `string | null` and manual `sendPayload` calls. This is the first step toward streaming support. I split `renderToResponseWithComponents` into a separate `renderToResponseWithComponentsInternal` function for ease of review: GitHub's diff rendering was highly misleading, making it seem as though more of the function had changed. The separate function just makes the actual change clearer: we split `renderToHTMLWithComponents` into two promises; one that represents the actual render result, and one that represents all of the work (including background work for e.g. revalidation) that needs to be done as part of generating the result. These changes make it easier to bubble up a `ResponsePayload`, instead of sometimes calling `sendPayload` out-of-band, centralizing all payload handling in `sendResponse` and eventually a similar function for public APIs that return a string. This centralization will make it much easier to handle a response that needs to be streamed, which is coming soon in another PR. --- packages/next/server/dev/next-dev-server.ts | 7 +- packages/next/server/next-server.ts | 205 ++++++++++++------ test/integration/prerender/test/index.test.js | 12 +- 3 files changed, 156 insertions(+), 68 deletions(-) diff --git a/packages/next/server/dev/next-dev-server.ts b/packages/next/server/dev/next-dev-server.ts index f95f1e8d7bfaa..48ad42876206d 100644 --- a/packages/next/server/dev/next-dev-server.ts +++ b/packages/next/server/dev/next-dev-server.ts @@ -30,6 +30,7 @@ import Server, { WrappedBuildError, ServerConstructor, FindComponentsResult, + ResponsePayload, } from '../next-server' import { normalizePagePath } from '../normalize-page-path' import Router, { Params, route } from '../router' @@ -636,14 +637,14 @@ export default class DevServer extends Server { return await loadDefaultErrorComponents(this.distDir) } - sendHTML( + sendResponse( req: IncomingMessage, res: ServerResponse, - html: string + response: ResponsePayload ): Promise { // In dev, we should not cache pages for any reason. res.setHeader('Cache-Control', 'no-store, must-revalidate') - return super.sendHTML(req, res, html) + return super.sendResponse(req, res, response) } protected setImmutableAssetCacheControl(res: ServerResponse): void { diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts index 28d8f0fdf95da..aaf63c16a711f 100644 --- a/packages/next/server/next-server.ts +++ b/packages/next/server/next-server.ts @@ -1291,16 +1291,23 @@ export default class Server { await this.render404(req, res, parsedUrl) } - protected async sendHTML( + protected async sendResponse( req: IncomingMessage, res: ServerResponse, - html: string + { type, body, revalidateOptions }: ResponsePayload ): Promise { const { generateEtags, poweredByHeader } = this.renderOpts - return sendPayload(req, res, html, 'html', { - generateEtags, - poweredByHeader, - }) + return sendPayload( + req, + res, + body, + type, + { + generateEtags, + poweredByHeader, + }, + revalidateOptions + ) } public async render( @@ -1350,13 +1357,13 @@ export default class Server { return this.render404(req, res, parsedUrl) } - const html = await this.renderToHTML(req, res, pathname, query) + const response = await this.renderToResponse(req, res, pathname, query) // Request was ended by the user - if (html === null) { + if (response === null) { return } - return this.sendHTML(req, res, html) + return this.sendResponse(req, res, response) } protected async findPageComponents( @@ -1443,13 +1450,53 @@ export default class Server { } } - private async renderToHTMLWithComponents( + private renderToResponseWithComponents( req: IncomingMessage, res: ServerResponse, pathname: string, - { components, query }: FindComponentsResult, + findComponentsResult: FindComponentsResult, opts: RenderOptsPartial - ): Promise { + ): Promise { + return new Promise(async (resolver, reject) => { + try { + let responded = false + return await this.renderToResponseWithComponentsInternal({ + req, + res, + pathname, + findComponentsResult, + opts, + respondWith: (response) => { + if (!responded) { + responded = true + resolver(response) + } + }, + hasResponded: () => responded || isResSent(res), + }) + } catch (err) { + reject(err) + } + }) + } + + private async renderToResponseWithComponentsInternal({ + req, + res, + pathname, + opts, + findComponentsResult: { components, query }, + respondWith, + hasResponded, + }: { + req: IncomingMessage + res: ServerResponse + pathname: string + findComponentsResult: FindComponentsResult + opts: RenderOptsPartial + respondWith: (response: ResponsePayload | null) => void + hasResponded: () => boolean + }): Promise { const is404Page = pathname === '/404' const is500Page = pathname === '/500' @@ -1478,7 +1525,11 @@ export default class Server { // handle static page if (typeof components.Component === 'string') { - return components.Component + respondWith({ + type: 'html', + body: components.Component, + }) + return } if (!query.amp) { @@ -1639,22 +1690,16 @@ export default class Server { } as UrlWithParsedQuery) } } else { - sendPayload( - req, - res, - data, - isDataReq ? 'json' : 'html', - { - generateEtags: this.renderOpts.generateEtags, - poweredByHeader: this.renderOpts.poweredByHeader, - }, - revalidateOptions - ) + respondWith({ + type: isDataReq ? 'json' : 'html', + body: data!, + revalidateOptions, + }) } // Stop the request chain here if the data we sent was up-to-date if (!cachedData.isStale) { - return null + return } } @@ -1758,7 +1803,7 @@ export default class Server { const isProduction = !this.renderOpts.dev const isDynamicPathname = isDynamicRoute(pathname) - const didRespond = isResSent(res) + const didRespond = hasResponded() const { staticPaths, fallbackMode } = hasStaticPaths ? await this.getStaticPaths(pathname) @@ -1825,11 +1870,11 @@ export default class Server { html = renderResult.html } - sendPayload(req, res, html, 'html', { - generateEtags: this.renderOpts.generateEtags, - poweredByHeader: this.renderOpts.poweredByHeader, + respondWith({ + type: 'html', + body: html, }) - return null + return } } @@ -1849,24 +1894,18 @@ export default class Server { : undefined if ( - !isResSent(res) && + !hasResponded() && !isNotFound && (isSSG || isDataReq || hasServerProps) ) { if (isRedirect && !isDataReq) { await handleRedirect(pageData) } else { - sendPayload( - req, - res, - isDataReq ? JSON.stringify(pageData) : html, - isDataReq ? 'json' : 'html', - { - generateEtags: this.renderOpts.generateEtags, - poweredByHeader: this.renderOpts.poweredByHeader, - }, - revalidateOptions - ) + respondWith({ + type: isDataReq ? 'json' : 'html', + body: isDataReq ? JSON.stringify(pageData) : html, + revalidateOptions, + }) } resHtml = null } @@ -1880,7 +1919,7 @@ export default class Server { ) } - if (!isResSent(res) && isNotFound) { + if (!hasResponded() && isNotFound) { if (revalidateOptions) { setRevalidateHeaders(res, revalidateOptions) } @@ -1894,15 +1933,17 @@ export default class Server { } as UrlWithParsedQuery) } } - return resHtml + respondWith( + resHtml ? { type: 'html', body: resHtml, revalidateOptions } : null + ) } - public async renderToHTML( + private async renderToResponse( req: IncomingMessage, res: ServerResponse, pathname: string, query: ParsedUrlQuery = {} - ): Promise { + ): Promise { const bubbleNoFallback = !!query._nextBubbleNoFallback delete query._nextBubbleNoFallback @@ -1910,7 +1951,7 @@ export default class Server { const result = await this.findPageComponents(pathname, query) if (result) { try { - return await this.renderToHTMLWithComponents( + return await this.renderToResponseWithComponents( req, res, pathname, @@ -1940,7 +1981,7 @@ export default class Server { ) if (dynamicRouteResult) { try { - return await this.renderToHTMLWithComponents( + return await this.renderToResponseWithComponents( req, res, dynamicRoute.page, @@ -1966,12 +2007,12 @@ export default class Server { } if (err instanceof DecodeError) { res.statusCode = 400 - return await this.renderErrorToHTML(err, req, res, pathname, query) + return await this.renderErrorToResponse(err, req, res, pathname, query) } res.statusCode = 500 const isWrappedError = err instanceof WrappedBuildError - const html = await this.renderErrorToHTML( + const response = await this.renderErrorToResponse( isWrappedError ? err.innerError : err, req, res, @@ -1985,10 +2026,20 @@ export default class Server { } this.logError(err) } - return html + return response } res.statusCode = 404 - return await this.renderErrorToHTML(null, req, res, pathname, query) + return this.renderErrorToResponse(null, req, res, pathname, query) + } + + public async renderToHTML( + req: IncomingMessage, + res: ServerResponse, + pathname: string, + query: ParsedUrlQuery = {} + ): Promise { + const response = await this.renderToResponse(req, res, pathname, query) + return response ? response.body : null } public async renderError( @@ -2005,15 +2056,21 @@ export default class Server { 'no-cache, no-store, max-age=0, must-revalidate' ) } - const html = await this.renderErrorToHTML(err, req, res, pathname, query) + const response = await this.renderErrorToResponse( + err, + req, + res, + pathname, + query + ) if (this.minimalMode && res.statusCode === 500) { throw err } - if (html === null) { + if (response === null) { return } - return this.sendHTML(req, res, html) + return this.sendResponse(req, res, response) } private customErrorNo404Warn = execOnce(() => { @@ -2025,13 +2082,13 @@ export default class Server { ) }) - public async renderErrorToHTML( + private async renderErrorToResponse( _err: Error | null, req: IncomingMessage, res: ServerResponse, _pathname: string, query: ParsedUrlQuery = {} - ) { + ): Promise { let err = _err if (this.renderOpts.dev && !err && res.statusCode === 500) { err = new Error( @@ -2039,7 +2096,6 @@ export default class Server { 'See https://nextjs.org/docs/messages/threw-undefined' ) } - let html: string | null try { let result: null | FindComponentsResult = null @@ -2072,7 +2128,7 @@ export default class Server { } try { - html = await this.renderToHTMLWithComponents( + return await this.renderToResponseWithComponents( req, res, statusPage, @@ -2097,7 +2153,7 @@ export default class Server { const fallbackComponents = await this.getFallbackErrorComponents() if (fallbackComponents) { - return this.renderToHTMLWithComponents( + return this.renderToResponseWithComponents( req, res, '/_error', @@ -2115,9 +2171,28 @@ export default class Server { } ) } - html = 'Internal Server Error' + return { + type: 'html', + body: 'Internal Server Error', + } } - return html + } + + public async renderErrorToHTML( + err: Error | null, + req: IncomingMessage, + res: ServerResponse, + pathname: string, + query: ParsedUrlQuery = {} + ): Promise { + const response = await this.renderErrorToResponse( + err, + req, + res, + pathname, + query + ) + return response ? response.body : null } protected async getFallbackErrorComponents(): Promise { @@ -2296,3 +2371,9 @@ export class WrappedBuildError extends Error { this.innerError = innerError } } + +export type ResponsePayload = { + type: 'html' | 'json' + body: string + revalidateOptions?: any +} diff --git a/test/integration/prerender/test/index.test.js b/test/integration/prerender/test/index.test.js index 3da6fabd156b8..afd2e8ecc9922 100644 --- a/test/integration/prerender/test/index.test.js +++ b/test/integration/prerender/test/index.test.js @@ -228,6 +228,10 @@ const expectedManifestRoutes = () => ({ }, }) +function isCachingHeader(cacheControl) { + return !cacheControl || !/no-store/.test(cacheControl) +} + const navigateTest = (dev = false) => { it('should navigate between pages successfully', async () => { const toBuild = [ @@ -974,18 +978,20 @@ const runTests = (dev = false, isEmulatedServerless = false) => { it('should always call getStaticProps without caching in dev', async () => { const initialRes = await fetchViaHTTP(appPort, '/something') - expect(initialRes.headers.get('cache-control')).toBeFalsy() + expect(isCachingHeader(initialRes.headers.get('cache-control'))).toBe( + false + ) const initialHtml = await initialRes.text() expect(initialHtml).toMatch(/hello.*?world/) const newRes = await fetchViaHTTP(appPort, '/something') - expect(newRes.headers.get('cache-control')).toBeFalsy() + expect(isCachingHeader(newRes.headers.get('cache-control'))).toBe(false) const newHtml = await newRes.text() expect(newHtml).toMatch(/hello.*?world/) expect(initialHtml !== newHtml).toBe(true) const newerRes = await fetchViaHTTP(appPort, '/something') - expect(newerRes.headers.get('cache-control')).toBeFalsy() + expect(isCachingHeader(newerRes.headers.get('cache-control'))).toBe(false) const newerHtml = await newerRes.text() expect(newerHtml).toMatch(/hello.*?world/) expect(newHtml !== newerHtml).toBe(true)