From 249be5d93e6bbf31d68dbeb5d89e2a2695b89451 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Fri, 9 Oct 2020 14:13:14 -0500 Subject: [PATCH 1/2] Add support for returning 404 from getStaticProps --- 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 | 9 +++- .../next-server/server/incremental-cache.ts | 20 ++++--- .../next/next-server/server/next-server.ts | 54 ++++++++++++++++--- packages/next/next-server/server/render.tsx | 10 +++- packages/next/server/next-dev-server.ts | 3 ++ 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 | 35 ++++++++++-- 13 files changed, 235 insertions(+), 35 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 e826708786e43..229ffd0cc2baa 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 } @@ -703,6 +704,7 @@ export default async function build( const finalPrerenderRoutes: { [route: string]: SsgRoute } = {} const tbdPrerenderRoutes: string[] = [] + let ssgNotFoundPaths: string[] = [] if (postCompileSpinner) postCompileSpinner.stopAndPersist() @@ -720,6 +722,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 @@ -812,6 +815,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) { @@ -865,11 +869,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 || !isSsg || additionalSsgFile) { + if ((!i18n || !isSsg || additionalSsgFile) && !isNotFound) { await promises.mkdir(path.dirname(dest), { recursive: true }) await promises.rename(orig, dest) } @@ -885,9 +890,14 @@ export default async function build( continue } + 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, @@ -907,9 +917,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) @@ -1060,6 +1068,7 @@ export default async function build( version: 2, routes: finalPrerenderRoutes, dynamicRoutes: finalDynamicRoutes, + notFoundRoutes: ssgNotFoundPaths, preview: previewProps, } @@ -1079,6 +1088,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 cc5f14a7a5ffc..c8f5ae4c20b66 100644 --- a/packages/next/export/index.ts +++ b/packages/next/export/index.ts @@ -467,13 +467,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 adc7a93d10c61..b28b36ced076b 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` @@ -395,6 +399,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 c6d0916aedd2f..84d8168199032 100644 --- a/packages/next/next-server/lib/router/router.ts +++ b/packages/next/next-server/lib/router/router.ts @@ -317,9 +317,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('NOT_FOUND') + } throw new Error(`Failed to load static props`) } - return res.json() }) } @@ -329,7 +333,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 === 'NOT_FOUND') { markLoadingError(err) } throw err diff --git a/packages/next/next-server/server/incremental-cache.ts b/packages/next/next-server/server/incremental-cache.ts index 3f1383935f4ae..0a4bf595d7c40 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 dfdba9b637a32..280679e7170fc 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -148,6 +148,7 @@ export default class Server { router: Router protected dynamicRoutes?: DynamicRoutes protected customRoutes: CustomRoutes + protected pendingSsg404s?: Set public constructor({ dir = '.', @@ -639,7 +640,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, @@ -684,7 +685,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, @@ -1165,12 +1166,31 @@ export default class Server { query.amp ? '.amp' : '' }` + // in development handle 404 before re-serving fallback + // since the page reloads when the data 404s and now + // we need to render the 404, in production this is + // populated in the incremental cache which is a no-op in dev + if ( + ssgCacheKey && + this.renderOpts.dev && + this.pendingSsg404s?.has(ssgCacheKey) + ) { + this.pendingSsg404s.delete(ssgCacheKey) + 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 @@ -1215,10 +1235,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 @@ -1234,9 +1256,15 @@ export default class Server { } ) + if (renderResult.renderOpts.ssgNotFound) { + // trigger 404 + throw new NoFallbackError() + } + 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({ @@ -1276,9 +1304,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 } } ) @@ -1360,10 +1389,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, @@ -1388,11 +1422,19 @@ export default class Server { if (isOrigin && ssgCacheKey) { await this.incrementalCache.set( ssgCacheKey, - { html: html!, pageData }, + { html: html!, pageData, isNotFound }, sprRevalidate ) } + if (isNotFound) { + // trigger 404 + if (ssgCacheKey && this.renderOpts.dev && this.pendingSsg404s) { + this.pendingSsg404s.add(ssgCacheKey) + } + throw new NoFallbackError() + } + return resHtml } diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index 332823e52612a..00a4efd07faa8 100644 --- a/packages/next/next-server/server/render.tsx +++ b/packages/next/next-server/server/render.tsx @@ -622,7 +622,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 +636,11 @@ export async function renderToHTML( throw new Error(invalidKeysMsg('getStaticProps', invalidKeys)) } + if (data.unstable_notFound) { + ;(renderOpts as any).ssgNotFound = true + return null + } + if ( data.unstable_redirect && typeof data.unstable_redirect === 'object' diff --git a/packages/next/server/next-dev-server.ts b/packages/next/server/next-dev-server.ts index d2b70721c3c0f..829ec2d18bd06 100644 --- a/packages/next/server/next-dev-server.ts +++ b/packages/next/server/next-dev-server.ts @@ -55,6 +55,7 @@ export default class DevServer extends Server { protected staticPathsWorker: import('jest-worker').default & { loadStaticPaths: typeof import('./static-paths-worker').loadStaticPaths } + protected pendingSsg404s: Set constructor(options: ServerConstructor & { isNextDevCommand?: boolean }) { super({ ...options, dev: true }) @@ -113,6 +114,8 @@ export default class DevServer extends Server { this.staticPathsWorker.getStdout().pipe(process.stdout) this.staticPathsWorker.getStderr().pipe(process.stderr) + + this.pendingSsg404s = new Set() } protected currentPhase(): string { diff --git a/packages/next/types/index.d.ts b/packages/next/types/index.d.ts index 3b26f7f10b386..e89990f38a859 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 8c573d748dcc0..7bb55bb36ccd5 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 0000000000000..e4e809bc4f321 --- /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 0000000000000..18a9bd7996f83 --- /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 117fb7bc9ffcd..f41345da79d91 100644 --- a/test/integration/i18n-support/test/index.test.js +++ b/test/integration/i18n-support/test/index.test.js @@ -22,11 +22,12 @@ 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', 'en'] -function runTests() { +function runTests(isDev) { it('should generate fallbacks with all locales', async () => { for (const locale of locales) { const html = await renderViaHTTP( @@ -58,7 +59,7 @@ function runTests() { } }) - 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) @@ -70,8 +71,30 @@ function runTests() { } }) - // 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) + } + }) + + // TODO: tests for notFound with fallback GSP pages it('should remove un-necessary locale prefix for default locale', async () => { const res = await fetchViaHTTP(appPort, '/en-US', undefined, { @@ -588,7 +611,7 @@ describe('i18n Support', () => { }) afterAll(() => killApp(app)) - runTests() + runTests(true) }) describe('production mode', () => { @@ -597,6 +620,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)) @@ -612,6 +636,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 () => { From 5866f1b2fa04e11c7a5fbc15e3a6f29644dcf506 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Sun, 11 Oct 2020 20:14:00 -0500 Subject: [PATCH 2/2] Add additional tests and update handling for dev --- .../next/next-server/lib/router/router.ts | 13 ++++- .../next/next-server/server/next-server.ts | 44 +++++--------- packages/next/next-server/server/render.tsx | 2 + packages/next/server/next-dev-server.ts | 3 - .../i18n-support/test/index.test.js | 58 ++++++++++++++++++- 5 files changed, 85 insertions(+), 35 deletions(-) diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts index 6992befa4d8ed..ad6805f1eb5d3 100644 --- a/packages/next/next-server/lib/router/router.ts +++ b/packages/next/next-server/lib/router/router.ts @@ -298,6 +298,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". @@ -320,7 +322,7 @@ function fetchRetry(url: string, attempts: number): Promise { 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('NOT_FOUND') + throw new Error(SSG_DATA_NOT_FOUND_ERROR) } throw new Error(`Failed to load static props`) } @@ -334,7 +336,7 @@ function fetchNextData(dataHref: string, isServerRender: boolean) { // on a client-side transition. Otherwise, we'd get into an infinite // loop. - if (!isServerRender || err.message === 'NOT_FOUND') { + if (!isServerRender || err.message === 'SSG Data NOT_FOUND') { markLoadingError(err) } throw err @@ -903,6 +905,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/next-server.ts b/packages/next/next-server/server/next-server.ts index 611704832c9b7..9ede0e4465de7 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -149,7 +149,6 @@ export default class Server { router: Router protected dynamicRoutes?: DynamicRoutes protected customRoutes: CustomRoutes - protected pendingSsg404s?: Set public constructor({ dir = '.', @@ -1077,6 +1076,7 @@ export default class Server { ...(components.getStaticProps ? { amp: query.amp, + __next404: query.__next404, _nextDataReq: query._nextDataReq, __nextLocale: query.__nextLocale, __nextLocales: query.__nextLocales, @@ -1151,6 +1151,13 @@ export default class Server { const isDataReq = !!query._nextDataReq && (isSSG || isServerProps) delete query._nextDataReq + const locale = query.__nextLocale as string + const locales = query.__nextLocales as string[] + // const defaultLocale = query.__nextDefaultLocale as string + delete query.__nextLocale + delete query.__nextLocales + // delete query.__nextDefaultLocale + let previewData: string | false | object | undefined let isPreviewMode = false @@ -1179,7 +1186,7 @@ export default class Server { } if (this.nextConfig.experimental.i18n) { - return normalizeLocalePath(path, this.renderOpts.locales).pathname + return normalizeLocalePath(path, locales).pathname } return path } @@ -1191,13 +1198,6 @@ export default class Server { urlPathname = stripNextDataPath(urlPathname) } - const locale = query.__nextLocale as string - const locales = query.__nextLocales as string[] - // const defaultLocale = query.__nextDefaultLocale as string - delete query.__nextLocale - delete query.__nextLocales - // delete query.__nextDefaultLocale - const ssgCacheKey = isPreviewMode || !isSSG ? undefined // Preview mode bypasses the cache @@ -1205,16 +1205,12 @@ export default class Server { query.amp ? '.amp' : '' }` - // in development handle 404 before re-serving fallback - // since the page reloads when the data 404s and now - // we need to render the 404, in production this is - // populated in the incremental cache which is a no-op in dev - if ( - ssgCacheKey && - this.renderOpts.dev && - this.pendingSsg404s?.has(ssgCacheKey) - ) { - this.pendingSsg404s.delete(ssgCacheKey) + // 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() } @@ -1296,11 +1292,6 @@ export default class Server { } ) - if (renderResult.renderOpts.ssgNotFound) { - // trigger 404 - throw new NoFallbackError() - } - html = renderResult.html pageData = renderResult.renderOpts.pageData sprRevalidate = renderResult.renderOpts.revalidate @@ -1470,13 +1461,8 @@ export default class Server { } if (isNotFound) { - // trigger 404 - if (ssgCacheKey && this.renderOpts.dev && this.pendingSsg404s) { - this.pendingSsg404s.add(ssgCacheKey) - } throw new NoFallbackError() } - return resHtml } diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index 479021db35840..591be3bb6a322 100644 --- a/packages/next/next-server/server/render.tsx +++ b/packages/next/next-server/server/render.tsx @@ -416,6 +416,7 @@ export async function renderToHTML( delete query.__nextLocale delete query.__nextLocales delete query.__nextDefaultLocale + delete query.__next404 const isSSG = !!getStaticProps const isBuildTimeSSG = isSSG && renderOpts.nextExport @@ -640,6 +641,7 @@ export async function renderToHTML( if (data.unstable_notFound) { ;(renderOpts as any).ssgNotFound = true + ;(renderOpts as any).revalidate = false return null } diff --git a/packages/next/server/next-dev-server.ts b/packages/next/server/next-dev-server.ts index 829ec2d18bd06..d2b70721c3c0f 100644 --- a/packages/next/server/next-dev-server.ts +++ b/packages/next/server/next-dev-server.ts @@ -55,7 +55,6 @@ export default class DevServer extends Server { protected staticPathsWorker: import('jest-worker').default & { loadStaticPaths: typeof import('./static-paths-worker').loadStaticPaths } - protected pendingSsg404s: Set constructor(options: ServerConstructor & { isNextDevCommand?: boolean }) { super({ ...options, dev: true }) @@ -114,8 +113,6 @@ export default class DevServer extends Server { this.staticPathsWorker.getStdout().pipe(process.stdout) this.staticPathsWorker.getStderr().pipe(process.stderr) - - this.pendingSsg404s = new Set() } protected currentPhase(): string { diff --git a/test/integration/i18n-support/test/index.test.js b/test/integration/i18n-support/test/index.test.js index 9066c8a3d420b..a5d520ca9e074 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) @@ -206,10 +207,65 @@ function runTests(isDev) { 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({}) + } } }) - // TODO: tests for notFound with fallback GSP pages + 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, {