From 41a5947aee82aef6d8623f68658631214e3abdf5 Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Sun, 1 Mar 2020 11:44:07 -0500 Subject: [PATCH 1/5] Correctly pass preview data --- .../next/next-server/server/next-server.ts | 45 +++++++------------ packages/next/next-server/server/render.tsx | 7 +-- 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 2717cf5218e8f..699740c000074 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -41,7 +41,7 @@ import pathMatch from './lib/path-match' import { recursiveReadDirSync } from './lib/recursive-readdir-sync' import { loadComponents, LoadComponentsReturnType } from './load-components' import { normalizePagePath } from './normalize-page-path' -import { renderToHTML } from './render' +import { RenderOpts, RenderOptsPartial, renderToHTML } from './render' import { getPagePath } from './require' import Router, { DynamicRoutes, @@ -115,6 +115,7 @@ export default class Server { documentMiddlewareEnabled: boolean hasCssMode: boolean dev?: boolean + previewProps: __ApiPreviewProps } private compression?: Middleware private onErrorMiddleware?: ({ err }: { err: Error }) => Promise @@ -165,6 +166,7 @@ export default class Server { staticMarkup, buildId: this.buildId, generateEtags, + previewProps: this.getPreviewProps(), } // Only the `publicRuntimeConfig` key is exposed to the client side @@ -668,13 +670,12 @@ export default class Server { } } - const previewProps = this.getPreviewProps() await apiResolver( req, res, query, pageModule, - { ...previewProps }, + this.renderOpts.previewProps, this.onErrorMiddleware ) return true @@ -780,7 +781,7 @@ export default class Server { return this.render404(req, res, parsedUrl) } - const html = await this.renderToHTML(req, res, pathname, query, {}) + const html = await this.renderToHTML(req, res, pathname, query) // Request was ended by the user if (html === null) { return @@ -869,7 +870,7 @@ export default class Server { res: ServerResponse, pathname: string, { components, query }: FindComponentsResult, - opts: any + opts: RenderOptsPartial ): Promise { // we need to ensure the status code if /404 is visited directly if (pathname === '/404') { @@ -890,7 +891,7 @@ export default class Server { const hasStaticPaths = !!components.getStaticPaths // Toggle whether or not this is a Data request - const isDataReq = query._nextDataReq + const isDataReq = !!query._nextDataReq delete query._nextDataReq // Serverless requests need its URL transformed back into the original @@ -958,8 +959,11 @@ export default class Server { }) } - const previewProps = this.getPreviewProps() - const previewData = tryGetPreviewData(req, res, { ...previewProps }) + const previewData = tryGetPreviewData( + req, + res, + this.renderOpts.previewProps + ) const isPreviewMode = previewData !== false // Compute the SPR cache key @@ -1017,7 +1021,7 @@ export default class Server { pageData = renderResult.renderOpts.pageData sprRevalidate = renderResult.renderOpts.revalidate } else { - const renderOpts = { + const renderOpts: RenderOpts = { ...components, ...opts, } @@ -1127,14 +1131,7 @@ export default class Server { req: IncomingMessage, res: ServerResponse, pathname: string, - query: ParsedUrlQuery = {}, - { - amphtml, - hasAmp, - }: { - amphtml?: boolean - hasAmp?: boolean - } = {} + query: ParsedUrlQuery = {} ): Promise { try { const result = await this.findPageComponents(pathname, query) @@ -1144,7 +1141,7 @@ export default class Server { res, pathname, result, - { ...this.renderOpts, amphtml, hasAmp } + { ...this.renderOpts } ) if (result2 !== false) { return result2 @@ -1169,12 +1166,7 @@ export default class Server { res, dynamicRoute.page, result, - { - ...this.renderOpts, - params, - amphtml, - hasAmp, - } + { ...this.renderOpts, params } ) if (result2 !== false) { return result2 @@ -1239,10 +1231,7 @@ export default class Server { res, using404Page ? '/404' : '/_error', result!, - { - ...this.renderOpts, - err, - } + { ...this.renderOpts, err } ) if (result2 === false) { throw new Error('invariant: failed to render error page') diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index 64e393d737178..9d9e96234132b 100644 --- a/packages/next/next-server/server/render.tsx +++ b/packages/next/next-server/server/render.tsx @@ -124,12 +124,11 @@ function render( return { html, head } } -type RenderOpts = LoadComponentsReturnType & { +export type RenderOptsPartial = { staticMarkup: boolean buildId: string canonicalBase: string runtimeConfig?: { [key: string]: any } - dangerousAsPath: string assetPrefix?: string hasCssMode: boolean err?: Error | null @@ -148,6 +147,8 @@ type RenderOpts = LoadComponentsReturnType & { previewProps: __ApiPreviewProps } +export type RenderOpts = LoadComponentsReturnType & RenderOptsPartial + function renderDocument( Document: DocumentType, { @@ -469,7 +470,7 @@ export async function renderToHTML( // Reads of this are cached on the `req` object, so this should resolve // instantly. There's no need to pass this data down from a previous // invoke, where we'd have to consider server & serverless. - const previewData = tryGetPreviewData(req, res, previewProps) + const previewData = tryGetPreviewData(req, res, previewProps) // TODO: null check const data = await getStaticProps!({ ...(pageIsDynamic ? { params: query as ParsedUrlQuery } : undefined), ...(previewData !== false From 61ced3b16b798f4eb8139b9fb6630f7ed480882a Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Sun, 1 Mar 2020 22:36:56 -0500 Subject: [PATCH 2/5] remove todo --- packages/next/next-server/server/render.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index 9d9e96234132b..20c2fcea21c63 100644 --- a/packages/next/next-server/server/render.tsx +++ b/packages/next/next-server/server/render.tsx @@ -470,7 +470,7 @@ export async function renderToHTML( // Reads of this are cached on the `req` object, so this should resolve // instantly. There's no need to pass this data down from a previous // invoke, where we'd have to consider server & serverless. - const previewData = tryGetPreviewData(req, res, previewProps) // TODO: null check + const previewData = tryGetPreviewData(req, res, previewProps) const data = await getStaticProps!({ ...(pageIsDynamic ? { params: query as ParsedUrlQuery } : undefined), ...(previewData !== false From 3c181da3d0ff41cbe5429746fb0c51f818f4cd13 Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Sun, 1 Mar 2020 22:37:27 -0500 Subject: [PATCH 3/5] re-do change --- packages/next/next-server/server/next-server.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 699740c000074..b888182df815c 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -1231,7 +1231,10 @@ export default class Server { res, using404Page ? '/404' : '/_error', result!, - { ...this.renderOpts, err } + { + ...this.renderOpts, + err, + } ) if (result2 === false) { throw new Error('invariant: failed to render error page') From 8b0fa28495e305b363e313e30b504cca21621192 Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Sun, 1 Mar 2020 22:39:25 -0500 Subject: [PATCH 4/5] fix types --- packages/next/next-server/server/next-server.ts | 5 +++-- packages/next/next-server/server/render.tsx | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index b888182df815c..5108dea904285 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -1028,8 +1028,9 @@ export default class Server { renderResult = await renderToHTML(req, res, pathname, query, renderOpts) html = renderResult - pageData = renderOpts.pageData - sprRevalidate = renderOpts.revalidate + // TODO: change this to a different passing mechanism + pageData = (renderOpts as any).pageData + sprRevalidate = (renderOpts as any).revalidate } return { html, pageData, sprRevalidate } diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index 20c2fcea21c63..e4b0d81dd3d48 100644 --- a/packages/next/next-server/server/render.tsx +++ b/packages/next/next-server/server/render.tsx @@ -519,6 +519,7 @@ export async function renderToHTML( props.pageProps = data.props // pass up revalidate and props for export + // TODO: change this to a different passing mechanism ;(renderOpts as any).revalidate = data.revalidate ;(renderOpts as any).pageData = props } From bcb33b4cc9e8b812f8effdbf613b082c28d218af Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Sun, 1 Mar 2020 23:56:04 -0500 Subject: [PATCH 5/5] Prevent regression --- packages/next/next-server/server/api-utils.ts | 10 +++ .../prerender-preview/test/index.test.js | 61 +++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/packages/next/next-server/server/api-utils.ts b/packages/next/next-server/server/api-utils.ts index 340d4babd71f2..3ccfba8773dea 100644 --- a/packages/next/next-server/server/api-utils.ts +++ b/packages/next/next-server/server/api-utils.ts @@ -260,6 +260,7 @@ const COOKIE_NAME_PRERENDER_BYPASS = `__prerender_bypass` const COOKIE_NAME_PRERENDER_DATA = `__next_preview_data` export const SYMBOL_PREVIEW_DATA = Symbol(COOKIE_NAME_PRERENDER_DATA) +const SYMBOL_CLEARED_COOKIES = Symbol(COOKIE_NAME_PRERENDER_BYPASS) export function tryGetPreviewData( req: IncomingMessage, @@ -405,6 +406,10 @@ function setPreviewData( } function clearPreviewData(res: NextApiResponse): NextApiResponse { + if (SYMBOL_CLEARED_COOKIES in res) { + return res + } + const { serialize } = require('cookie') as typeof import('cookie') const previous = res.getHeader('Set-Cookie') res.setHeader(`Set-Cookie`, [ @@ -432,6 +437,11 @@ function clearPreviewData(res: NextApiResponse): NextApiResponse { path: '/', }), ]) + + Object.defineProperty(res, SYMBOL_CLEARED_COOKIES, { + value: true, + enumerable: false, + }) return res } diff --git a/test/integration/prerender-preview/test/index.test.js b/test/integration/prerender-preview/test/index.test.js index 51046b0d59b73..89e72e411be6d 100644 --- a/test/integration/prerender-preview/test/index.test.js +++ b/test/integration/prerender-preview/test/index.test.js @@ -8,6 +8,7 @@ import { findPort, initNextServerScript, killApp, + launchApp, nextBuild, nextStart, renderViaHTTP, @@ -201,6 +202,66 @@ const startServerlessEmulator = async (dir, port) => { } describe('Prerender Preview Mode', () => { + describe('Development Mode', () => { + beforeAll(async () => { + await fs.remove(nextConfigPath) + }) + + let appPort, app + it('should start development application', async () => { + appPort = await findPort() + app = await launchApp(appDir, appPort) + }) + + let previewCookieString + it('should enable preview mode', async () => { + const res = await fetchViaHTTP(appPort, '/api/preview', { lets: 'goooo' }) + expect(res.status).toBe(200) + + const cookies = res.headers + .get('set-cookie') + .split(',') + .map(cookie.parse) + + expect(cookies.length).toBe(2) + previewCookieString = + cookie.serialize('__prerender_bypass', cookies[0].__prerender_bypass) + + '; ' + + cookie.serialize('__next_preview_data', cookies[1].__next_preview_data) + }) + + it('should return cookies to be expired after dev server reboot', async () => { + await killApp(app) + app = await launchApp(appDir, appPort) + + const res = await fetchViaHTTP( + appPort, + '/', + {}, + { headers: { Cookie: previewCookieString } } + ) + expect(res.status).toBe(200) + + const body = await res.text() + // "err":{"name":"TypeError","message":"Cannot read property 'previewModeId' of undefined" + expect(body).not.toContain('err') + expect(body).not.toContain('TypeError') + expect(body).not.toContain('previewModeId') + + const cookies = res.headers + .get('set-cookie') + .replace(/(=\w{3}),/g, '$1') + .split(',') + .map(cookie.parse) + + expect(cookies.length).toBe(2) + }) + + afterAll(async () => { + await killApp(app) + }) + }) + describe('Server Mode', () => { beforeAll(async () => { await fs.remove(nextConfigPath)