From 2a94ae037a1a771cccd905a29c32fd67608d4cf2 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Thu, 15 Oct 2020 16:55:38 -0500 Subject: [PATCH] Add support for returning 404 from getStaticProps (#17755) --- packages/next/build/index.ts | 18 +++- packages/next/export/index.ts | 16 ++-- packages/next/export/worker.ts | 16 +++- .../next/next-server/lib/router/router.ts | 18 +++- .../next-server/server/incremental-cache.ts | 20 +++-- .../next/next-server/server/next-server.ts | 40 +++++++-- packages/next/next-server/server/render.tsx | 12 ++- packages/next/types/index.d.ts | 1 + .../i18n-support/pages/gsp/index.js | 1 - .../pages/not-found/fallback/[slug].js | 50 +++++++++++ .../i18n-support/pages/not-found/index.js | 37 ++++++++ .../i18n-support/test/index.test.js | 87 ++++++++++++++++++- 12 files changed, 283 insertions(+), 33 deletions(-) create mode 100644 test/integration/i18n-support/pages/not-found/fallback/[slug].js create mode 100644 test/integration/i18n-support/pages/not-found/index.js diff --git a/packages/next/build/index.ts b/packages/next/build/index.ts index dd87c15fede9e88..b497bf36ddc5201 100644 --- a/packages/next/build/index.ts +++ b/packages/next/build/index.ts @@ -96,6 +96,7 @@ export type PrerenderManifest = { version: 2 routes: { [route: string]: SsgRoute } dynamicRoutes: { [route: string]: DynamicSsgRoute } + notFoundRoutes: string[] preview: __ApiPreviewProps } @@ -713,6 +714,7 @@ export default async function build( const finalPrerenderRoutes: { [route: string]: SsgRoute } = {} const tbdPrerenderRoutes: string[] = [] + let ssgNotFoundPaths: string[] = [] if (postCompileSpinner) postCompileSpinner.stopAndPersist() @@ -730,6 +732,7 @@ export default async function build( const exportConfig: any = { ...config, initialPageRevalidationMap: {}, + ssgNotFoundPaths: [] as string[], // Default map will be the collection of automatic statically exported // pages and incremental pages. // n.b. we cannot handle this above in combinedPages because the dynamic @@ -821,6 +824,7 @@ export default async function build( const postBuildSpinner = createSpinner({ prefixText: `${Log.prefixes.info} Finalizing page optimization`, }) + ssgNotFoundPaths = exportConfig.ssgNotFoundPaths // remove server bundles that were exported for (const page of staticPages) { @@ -874,11 +878,12 @@ export default async function build( } const { i18n } = config.experimental + const isNotFound = ssgNotFoundPaths.includes(page) // for SSG files with i18n the non-prerendered variants are // output with the locale prefixed so don't attempt moving // without the prefix - if (!i18n || additionalSsgFile) { + if ((!i18n || additionalSsgFile) && !isNotFound) { await promises.mkdir(path.dirname(dest), { recursive: true }) await promises.rename(orig, dest) } else if (i18n && !isSsg) { @@ -891,9 +896,14 @@ export default async function build( if (additionalSsgFile) return for (const locale of i18n.locales) { + const curPath = `/${locale}${page === '/' ? '' : page}` const localeExt = page === '/' ? path.extname(file) : '' const relativeDestNoPages = relativeDest.substr('pages/'.length) + if (isSsg && ssgNotFoundPaths.includes(curPath)) { + continue + } + const updatedRelativeDest = path.join( 'pages', locale + localeExt, @@ -913,9 +923,7 @@ export default async function build( ) if (!isSsg) { - pagesManifest[ - `/${locale}${page === '/' ? '' : page}` - ] = updatedRelativeDest + pagesManifest[curPath] = updatedRelativeDest } await promises.mkdir(path.dirname(updatedDest), { recursive: true }) await promises.rename(updatedOrig, updatedDest) @@ -1066,6 +1074,7 @@ export default async function build( version: 2, routes: finalPrerenderRoutes, dynamicRoutes: finalDynamicRoutes, + notFoundRoutes: ssgNotFoundPaths, preview: previewProps, } @@ -1085,6 +1094,7 @@ export default async function build( routes: {}, dynamicRoutes: {}, preview: previewProps, + notFoundRoutes: [], } await promises.writeFile( path.join(distDir, PRERENDER_MANIFEST), diff --git a/packages/next/export/index.ts b/packages/next/export/index.ts index 318ff6e920fdf67..5c146431c303893 100644 --- a/packages/next/export/index.ts +++ b/packages/next/export/index.ts @@ -473,13 +473,17 @@ export default async function exportApp( renderError = renderError || !!result.error if (!!result.error) errorPaths.push(path) - if ( - options.buildExport && - typeof result.fromBuildExportRevalidate !== 'undefined' - ) { - configuration.initialPageRevalidationMap[path] = - result.fromBuildExportRevalidate + if (options.buildExport) { + if (typeof result.fromBuildExportRevalidate !== 'undefined') { + configuration.initialPageRevalidationMap[path] = + result.fromBuildExportRevalidate + } + + if (result.ssgNotFound === true) { + configuration.ssgNotFoundPaths.push(path) + } } + if (progress) progress() }) ) diff --git a/packages/next/export/worker.ts b/packages/next/export/worker.ts index 379121835242f7e..2fdb7da9cc9a76e 100644 --- a/packages/next/export/worker.ts +++ b/packages/next/export/worker.ts @@ -55,6 +55,7 @@ interface ExportPageResults { ampValidations: AmpValidation[] fromBuildExportRevalidate?: number error?: boolean + ssgNotFound?: boolean } interface RenderOpts { @@ -252,11 +253,11 @@ export default async function exportPage({ // @ts-ignore params ) - curRenderOpts = result.renderOpts || {} - html = result.html + curRenderOpts = (result as any).renderOpts || {} + html = (result as any).html } - if (!html) { + if (!html && !(curRenderOpts as any).ssgNotFound) { throw new Error(`Failed to render serverless page`) } } else { @@ -311,6 +312,7 @@ export default async function exportPage({ html = await renderMethod(req, res, page, query, curRenderOpts) } } + results.ssgNotFound = (curRenderOpts as any).ssgNotFound const validateAmp = async ( rawAmpHtml: string, @@ -334,7 +336,9 @@ export default async function exportPage({ } if (curRenderOpts.inAmpMode && !curRenderOpts.ampSkipValidation) { - await validateAmp(html, path, curRenderOpts.ampValidatorPath) + if (!results.ssgNotFound) { + await validateAmp(html, path, curRenderOpts.ampValidatorPath) + } } else if (curRenderOpts.hybridAmp) { // we need to render the AMP version let ampHtmlFilename = `${ampPath}${sep}index.html` @@ -396,6 +400,10 @@ export default async function exportPage({ } results.fromBuildExportRevalidate = (curRenderOpts as any).revalidate + if (results.ssgNotFound) { + // don't attempt writing to disk if getStaticProps returned not found + return results + } await promises.writeFile(htmlFilepath, html, 'utf8') return results } catch (error) { diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts index 240b206dcc1c231..85921f4d6ca8b1c 100644 --- a/packages/next/next-server/lib/router/router.ts +++ b/packages/next/next-server/lib/router/router.ts @@ -299,6 +299,8 @@ const manualScrollRestoration = typeof window !== 'undefined' && 'scrollRestoration' in window.history +const SSG_DATA_NOT_FOUND_ERROR = 'SSG Data NOT_FOUND' + function fetchRetry(url: string, attempts: number): Promise { return fetch(url, { // Cookies are required to be present for Next.js' SSG "Preview Mode". @@ -318,9 +320,13 @@ function fetchRetry(url: string, attempts: number): Promise { if (attempts > 1 && res.status >= 500) { return fetchRetry(url, attempts - 1) } + if (res.status === 404) { + // TODO: handle reloading in development from fallback returning 200 + // to on-demand-entry-handler causing it to reload periodically + throw new Error(SSG_DATA_NOT_FOUND_ERROR) + } throw new Error(`Failed to load static props`) } - return res.json() }) } @@ -330,7 +336,8 @@ function fetchNextData(dataHref: string, isServerRender: boolean) { // We should only trigger a server-side transition if this was caused // on a client-side transition. Otherwise, we'd get into an infinite // loop. - if (!isServerRender) { + + if (!isServerRender || err.message === 'SSG Data NOT_FOUND') { markLoadingError(err) } throw err @@ -900,6 +907,13 @@ export default class Router implements BaseRouter { // 3. Internal error while loading the page // So, doing a hard reload is the proper way to deal with this. + if (process.env.NODE_ENV === 'development') { + // append __next404 query to prevent fallback from being re-served + // on reload in development + if (err.message === SSG_DATA_NOT_FOUND_ERROR && this.isSsr) { + as += `${as.indexOf('?') > -1 ? '&' : '?'}__next404=1` + } + } window.location.href = as // Changing the URL doesn't block executing the current code path. diff --git a/packages/next/next-server/server/incremental-cache.ts b/packages/next/next-server/server/incremental-cache.ts index 3f1383935f4aedb..0a4bf595d7c40e9 100644 --- a/packages/next/next-server/server/incremental-cache.ts +++ b/packages/next/next-server/server/incremental-cache.ts @@ -10,9 +10,10 @@ function toRoute(pathname: string): string { } type IncrementalCacheValue = { - html: string - pageData: any + html?: string + pageData?: any isStale?: boolean + isNotFound?: boolean curRevalidate?: number | false // milliseconds to revalidate after revalidateAfter: number | false @@ -55,6 +56,7 @@ export class IncrementalCache { version: -1 as any, // letting us know this doesn't conform to spec routes: {}, dynamicRoutes: {}, + notFoundRoutes: [], preview: null as any, // `preview` is special case read in next-dev-server } } else { @@ -67,8 +69,9 @@ export class IncrementalCache { // default to 50MB limit max: max || 50 * 1024 * 1024, length(val) { + if (val.isNotFound) return 25 // rough estimate of size of cache value - return val.html.length + JSON.stringify(val.pageData).length + return val.html!.length + JSON.stringify(val.pageData).length }, }) } @@ -112,6 +115,10 @@ export class IncrementalCache { // let's check the disk for seed data if (!data) { + if (this.prerenderManifest.notFoundRoutes.includes(pathname)) { + return { isNotFound: true, revalidateAfter: false } + } + try { const html = await promises.readFile( this.getSeedPath(pathname, 'html'), @@ -151,8 +158,9 @@ export class IncrementalCache { async set( pathname: string, data: { - html: string - pageData: any + html?: string + pageData?: any + isNotFound?: boolean }, revalidateSeconds?: number | false ) { @@ -178,7 +186,7 @@ export class IncrementalCache { // TODO: This option needs to cease to exist unless it stops mutating the // `next build` output's manifest. - if (this.incrementalOptions.flushToDisk) { + if (this.incrementalOptions.flushToDisk && !data.isNotFound) { try { const seedPath = this.getSeedPath(pathname, 'html') await promises.mkdir(path.dirname(seedPath), { recursive: true }) diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 4c2d6244ba24bba..4e4a131189a0ecf 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -699,7 +699,7 @@ export default class Server { ) const { query } = parsedDestination - delete parsedDestination.query + delete (parsedDestination as any).query parsedDestination.search = stringifyQs(query, undefined, undefined, { encodeURIComponent: (str: string) => str, @@ -744,7 +744,7 @@ export default class Server { // external rewrite, proxy it if (parsedDestination.protocol) { const { query } = parsedDestination - delete parsedDestination.query + delete (parsedDestination as any).query parsedDestination.search = stringifyQs( query, undefined, @@ -1115,6 +1115,7 @@ export default class Server { ...(components.getStaticProps ? { amp: query.amp, + __next404: query.__next404, _nextDataReq: query._nextDataReq, __nextLocale: query.__nextLocale, } @@ -1240,12 +1241,27 @@ export default class Server { query.amp ? '.amp' : '' }` + // In development we use a __next404 query to allow signaling we should + // render the 404 page after attempting to fetch the _next/data for a + // fallback page since the fallback page will always be available after + // reload and we don't want to re-serve it and instead want to 404. + if (this.renderOpts.dev && isSSG && query.__next404) { + delete query.__next404 + throw new NoFallbackError() + } + // Complete the response with cached data if its present const cachedData = ssgCacheKey ? await this.incrementalCache.get(ssgCacheKey) : undefined if (cachedData) { + if (cachedData.isNotFound) { + // we don't currently revalidate when notFound is returned + // so trigger rendering 404 here + throw new NoFallbackError() + } + const data = isDataReq ? JSON.stringify(cachedData.pageData) : cachedData.html @@ -1290,10 +1306,12 @@ export default class Server { html: string | null pageData: any sprRevalidate: number | false + isNotFound?: boolean }> => { let pageData: any let html: string | null let sprRevalidate: number | false + let isNotFound: boolean | undefined let renderResult // handle serverless @@ -1313,6 +1331,7 @@ export default class Server { html = renderResult.html pageData = renderResult.renderOpts.pageData sprRevalidate = renderResult.renderOpts.revalidate + isNotFound = renderResult.renderOpts.ssgNotFound } else { const origQuery = parseUrl(req.url || '', true).query const resolvedUrl = formatUrl({ @@ -1354,9 +1373,10 @@ export default class Server { // TODO: change this to a different passing mechanism pageData = (renderOpts as any).pageData sprRevalidate = (renderOpts as any).revalidate + isNotFound = (renderOpts as any).ssgNotFound } - return { html, pageData, sprRevalidate } + return { html, pageData, sprRevalidate, isNotFound } } ) @@ -1438,10 +1458,15 @@ export default class Server { const { isOrigin, - value: { html, pageData, sprRevalidate }, + value: { html, pageData, sprRevalidate, isNotFound }, } = await doRender() let resHtml = html - if (!isResSent(res) && (isSSG || isDataReq || isServerProps)) { + + if ( + !isResSent(res) && + !isNotFound && + (isSSG || isDataReq || isServerProps) + ) { sendPayload( req, res, @@ -1466,11 +1491,14 @@ export default class Server { if (isOrigin && ssgCacheKey) { await this.incrementalCache.set( ssgCacheKey, - { html: html!, pageData }, + { html: html!, pageData, isNotFound }, sprRevalidate ) } + if (isNotFound) { + throw new NoFallbackError() + } return resHtml } diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index 332823e52612af0..a22f6904a67cdb9 100644 --- a/packages/next/next-server/server/render.tsx +++ b/packages/next/next-server/server/render.tsx @@ -414,6 +414,7 @@ export async function renderToHTML( const isFallback = !!query.__nextFallback delete query.__nextFallback delete query.__nextLocale + delete query.__next404 const isSSG = !!getStaticProps const isBuildTimeSSG = isSSG && renderOpts.nextExport @@ -622,7 +623,10 @@ export async function renderToHTML( const invalidKeys = Object.keys(data).filter( (key) => - key !== 'revalidate' && key !== 'props' && key !== 'unstable_redirect' + key !== 'revalidate' && + key !== 'props' && + key !== 'unstable_redirect' && + key !== 'unstable_notFound' ) if (invalidKeys.includes('unstable_revalidate')) { @@ -633,6 +637,12 @@ export async function renderToHTML( throw new Error(invalidKeysMsg('getStaticProps', invalidKeys)) } + if (data.unstable_notFound) { + ;(renderOpts as any).ssgNotFound = true + ;(renderOpts as any).revalidate = false + return null + } + if ( data.unstable_redirect && typeof data.unstable_redirect === 'object' diff --git a/packages/next/types/index.d.ts b/packages/next/types/index.d.ts index 3b26f7f10b386ea..e89990f38a8598f 100644 --- a/packages/next/types/index.d.ts +++ b/packages/next/types/index.d.ts @@ -89,6 +89,7 @@ export type GetStaticPropsResult

= { props?: P revalidate?: number | boolean unstable_redirect?: Redirect + unstable_notFound?: true } export type GetStaticProps< diff --git a/test/integration/i18n-support/pages/gsp/index.js b/test/integration/i18n-support/pages/gsp/index.js index 8c573d748dcc06a..7bb55bb36ccd547 100644 --- a/test/integration/i18n-support/pages/gsp/index.js +++ b/test/integration/i18n-support/pages/gsp/index.js @@ -21,7 +21,6 @@ export default function Page(props) { ) } -// TODO: should non-dynamic GSP pages pre-render for each locale? export const getStaticProps = ({ locale, locales }) => { return { props: { diff --git a/test/integration/i18n-support/pages/not-found/fallback/[slug].js b/test/integration/i18n-support/pages/not-found/fallback/[slug].js new file mode 100644 index 000000000000000..e4e809bc4f32137 --- /dev/null +++ b/test/integration/i18n-support/pages/not-found/fallback/[slug].js @@ -0,0 +1,50 @@ +import Link from 'next/link' +import { useRouter } from 'next/router' + +export default function Page(props) { + const router = useRouter() + + if (router.isFallback) return 'Loading...' + + return ( + <> +

gsp page

+

{JSON.stringify(props)}

+

{router.locale}

+

{JSON.stringify(router.locales)}

+

{JSON.stringify(router.query)}

+

{router.pathname}

+

{router.asPath}

+ + to / + +
+ + ) +} + +export const getStaticProps = ({ params, locale, locales }) => { + if (locale === 'en' || locale === 'nl') { + return { + unstable_notFound: true, + } + } + + return { + props: { + params, + locale, + locales, + }, + } +} + +export const getStaticPaths = () => { + return { + // the default locale will be used since one isn't defined here + paths: ['first', 'second'].map((slug) => ({ + params: { slug }, + })), + fallback: true, + } +} diff --git a/test/integration/i18n-support/pages/not-found/index.js b/test/integration/i18n-support/pages/not-found/index.js new file mode 100644 index 000000000000000..18a9bd7996f8329 --- /dev/null +++ b/test/integration/i18n-support/pages/not-found/index.js @@ -0,0 +1,37 @@ +import Link from 'next/link' +import { useRouter } from 'next/router' + +export default function Page(props) { + const router = useRouter() + + return ( + <> +

gsp page

+

{JSON.stringify(props)}

+

{router.locale}

+

{JSON.stringify(router.locales)}

+

{JSON.stringify(router.query)}

+

{router.pathname}

+

{router.asPath}

+ + to / + +
+ + ) +} + +export const getStaticProps = ({ locale, locales }) => { + if (locale === 'en' || locale === 'nl') { + return { + unstable_notFound: true, + } + } + + return { + props: { + locale, + locales, + }, + } +} diff --git a/test/integration/i18n-support/test/index.test.js b/test/integration/i18n-support/test/index.test.js index 565e910b8eff8c5..e4763437a661e2d 100644 --- a/test/integration/i18n-support/test/index.test.js +++ b/test/integration/i18n-support/test/index.test.js @@ -14,6 +14,7 @@ import { nextStart, renderViaHTTP, File, + waitFor, } from 'next-test-utils' jest.setTimeout(1000 * 60 * 2) @@ -22,6 +23,7 @@ const appDir = join(__dirname, '../') const nextConfig = new File(join(appDir, 'next.config.js')) let app let appPort +let buildPagesDir // let buildId const locales = ['en-US', 'nl-NL', 'nl-BE', 'nl', 'fr-BE', 'fr', 'en'] @@ -447,7 +449,7 @@ function runTests(isDev) { } }) - it('should generate non-dynamic SSG page with all locales', async () => { + it('should generate non-dynamic GSP page with all locales', async () => { for (const locale of locales) { const html = await renderViaHTTP(appPort, `/${locale}/gsp`) const $ = cheerio.load(html) @@ -468,8 +470,85 @@ function runTests(isDev) { } }) - // TODO: SSG 404 behavior to opt-out of generating specific locale - // for non-dynamic SSG pages + if (!isDev) { + it('should not output GSP pages that returned notFound', async () => { + const skippedLocales = ['en', 'nl'] + + for (const locale of locales) { + const pagePath = join(buildPagesDir, locale, 'not-found.html') + const dataPath = join(buildPagesDir, locale, 'not-found.json') + console.log(pagePath) + expect(await fs.exists(pagePath)).toBe(!skippedLocales.includes(locale)) + expect(await fs.exists(dataPath)).toBe(!skippedLocales.includes(locale)) + } + }) + } + + it('should 404 for GSP pages that returned notFound', async () => { + const skippedLocales = ['en', 'nl'] + + for (const locale of locales) { + const res = await fetchViaHTTP(appPort, `/${locale}/not-found`) + expect(res.status).toBe(skippedLocales.includes(locale) ? 404 : 200) + + if (skippedLocales.includes(locale)) { + const browser = await webdriver(appPort, `/${locale}/not-found`) + expect(await browser.elementByCss('html').getAttribute('lang')).toBe( + locale + ) + expect( + await browser.eval('document.documentElement.innerHTML') + ).toContain('This page could not be found') + + const parsedUrl = url.parse( + await browser.eval('window.location.href'), + true + ) + expect(parsedUrl.pathname).toBe(`/${locale}/not-found`) + expect(parsedUrl.query).toEqual({}) + } + } + }) + + it('should 404 for GSP that returned notFound on client-transition', async () => { + const browser = await webdriver(appPort, '/en') + await browser.eval(`(function() { + window.beforeNav = 1 + window.next.router.push('/not-found') + })()`) + + await browser.waitForElementByCss('h1') + + expect(await browser.elementByCss('html').getAttribute('lang')).toBe('en') + expect(await browser.elementByCss('html').text()).toContain( + 'This page could not be found' + ) + expect(await browser.eval('window.beforeNav')).toBe(null) + }) + + it('should render 404 for fallback page that returned 404', async () => { + const browser = await webdriver(appPort, '/en/not-found/fallback/first') + await browser.waitForElementByCss('h1') + await browser.eval('window.beforeNav = 1') + + expect(await browser.elementByCss('html').text()).toContain( + 'This page could not be found' + ) + expect(await browser.elementByCss('html').getAttribute('lang')).toBe('en') + + const parsedUrl = url.parse( + await browser.eval('window.location.href'), + true + ) + expect(parsedUrl.pathname).toBe('/en/not-found/fallback/first') + expect(parsedUrl.query).toEqual(isDev ? { __next404: '1' } : {}) + + if (isDev) { + // make sure page doesn't reload un-necessarily in development + await waitFor(10 * 1000) + } + expect(await browser.eval('window.beforeNav')).toBe(1) + }) it('should remove un-necessary locale prefix for default locale', async () => { const res = await fetchViaHTTP(appPort, '/en-US', undefined, { @@ -1018,6 +1097,7 @@ describe('i18n Support', () => { await nextBuild(appDir) appPort = await findPort() app = await nextStart(appDir, appPort) + buildPagesDir = join(appDir, '.next/server/pages') // buildId = await fs.readFile(join(appDir, '.next/BUILD_ID'), 'utf8') }) afterAll(() => killApp(app)) @@ -1033,6 +1113,7 @@ describe('i18n Support', () => { await nextBuild(appDir) appPort = await findPort() app = await nextStart(appDir, appPort) + buildPagesDir = join(appDir, '.next/serverless/pages') // buildId = await fs.readFile(join(appDir, '.next/BUILD_ID'), 'utf8') }) afterAll(async () => {