Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reuse of inline flight response and 404 for RSC in node runtime #34202

Merged
merged 2 commits into from Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/next/client/index.tsx
Expand Up @@ -744,7 +744,7 @@ if (process.env.__NEXT_RSC) {
let response = rscCache.get(cacheKey)
if (response) return response

const bufferCacheKey = cacheKey + ',' + id
const bufferCacheKey = cacheKey + ',' + router.route + ',' + id
if (serverDataBuffer.has(bufferCacheKey)) {
const t = new TransformStream()
const writer = t.writable.getWriter()
Expand Down
2 changes: 1 addition & 1 deletion packages/next/client/page-loader.ts
Expand Up @@ -148,7 +148,7 @@ export default class PageLoader {

const getHrefForSlug = (path: string) => {
if (rsc) {
return path + search + (search ? `&` : '?') + '__flight__'
return path + search + (search ? `&` : '?') + '__flight__=1'
}

const dataRoute = getAssetPathFromRoute(
Expand Down
6 changes: 5 additions & 1 deletion packages/next/server/base-server.ts
Expand Up @@ -1126,9 +1126,13 @@ export default abstract class Server {
// Toggle whether or not this is a Data request
const isDataReq = !!query._nextDataReq && (isSSG || hasServerProps)
delete query._nextDataReq
// Don't delete query.__flight__ yet, it still needs to be used in renderToHTML later
const isFlightRequest = Boolean(
this.serverComponentManifest && query.__flight__
)

// we need to ensure the status code if /404 is visited directly
if (is404Page && !isDataReq) {
if (is404Page && !isDataReq && !isFlightRequest) {
res.statusCode = 404
}

Expand Down
3 changes: 2 additions & 1 deletion packages/next/server/render.tsx
Expand Up @@ -477,8 +477,9 @@ export async function renderToHTML(

if (isServerComponent) {
serverComponentsInlinedTransformStream = new TransformStream()
const search = stringifyQuery(query)
Component = createServerComponentRenderer(App, OriginalComponent, {
cachePrefix: pathname + '?' + stringifyQuery(query),
cachePrefix: pathname + (search ? `?${search}` : ''),
transformStream: serverComponentsInlinedTransformStream,
serverComponentManifest,
runtime,
Expand Down
Expand Up @@ -17,8 +17,7 @@ function Data() {
export default function Page404() {
return (
<Suspense fallback={null}>
custom-404-page
<br />
<span id="text">custom-404-page</span>
<Data />
</Suspense>
)
Expand Down
@@ -1,6 +1,7 @@
import webdriver from 'next-webdriver'
import { renderViaHTTP } from 'next-test-utils'

export default async function basic(context, env) {
export default async function basic(context) {
it('should render 404 error correctly', async () => {
const path404HTML = await renderViaHTTP(context.appPort, '/404')
const pathNotFoundHTML = await renderViaHTTP(context.appPort, '/not-found')
Expand All @@ -27,4 +28,11 @@ export default async function basic(context, env) {
const res = await renderViaHTTP(context.appPort, '/api/ping')
expect(res).toContain('pong')
})

it('should handle suspense error page correctly (node stream)', async () => {
const browser = await webdriver(context.appPort, '/404')
const hydrationContent = await browser.waitForElementByCss('#__next').text()

expect(hydrationContent).toBe('custom-404-pagenext_streaming_data')
})
}
Expand Up @@ -27,7 +27,6 @@ const documentPage = new File(join(appDir, 'pages/_document.jsx'))
const appPage = new File(join(appDir, 'pages/_app.js'))
const appServerPage = new File(join(appDir, 'pages/_app.server.js'))
const error500Page = new File(join(appDir, 'pages/500.js'))
const error404Page = new File(join(appDir, 'pages/404.js'))
const nextConfig = new File(join(appDir, 'next.config.js'))

const documentWithGip = `
Expand Down Expand Up @@ -73,33 +72,6 @@ export default function Page500() {
}
`

const suspense404 = `
import { Suspense } from 'react'

let result
let promise
function Data() {
if (result) return result
if (!promise)
promise = new Promise((res) => {
setTimeout(() => {
result = 'next_streaming_data'
res()
}, 500)
})
throw promise
}

export default function Page404() {
return (
<Suspense fallback={null}>
custom-404-page
<Data />
</Suspense>
)
}
`

describe('Edge runtime - basic', () => {
it('should warn user for experimental risk with server components', async () => {
const edgeRuntimeWarning =
Expand All @@ -116,20 +88,6 @@ describe('Edge runtime - basic', () => {
const { stderr } = await nextBuild(nativeModuleTestAppDir)
expect(stderr).toContain(fsImportedErrorMessage)
})

it('should handle suspense error page correctly (node stream)', async () => {
error404Page.write(suspense404)
const appPort = await findPort()
await nextBuild(appDir)
await nextStart(appDir, appPort)
const browser = await webdriver(appPort, '/404')
const hydrationContent = await browser.eval(
`document.querySelector('#__next').textContent`
)
expect(hydrationContent).toBe('custom-404-pagenext_streaming_data')

error404Page.restore()
})
})

describe('Edge runtime - prod', () => {
Expand Down Expand Up @@ -200,8 +158,8 @@ describe('Edge runtime - prod', () => {
expect(path500HTML).toContain('custom-500-page')
})

basic(context, 'prod')
rsc(context)
basic(context)
rsc(context, 'edge')
streaming(context)
})

Expand Down Expand Up @@ -246,15 +204,16 @@ describe('Edge runtime - dev', () => {
expect(path500HTML).toContain('Error: oops')
})

basic(context, 'dev')
rsc(context)
basic(context)
rsc(context, 'edge')
streaming(context)
})

const nodejsRuntimeBasicSuite = {
runTests: (context, env) => {
basic(context, env)
basic(context)
streaming(context)
rsc(context, 'nodejs')

if (env === 'prod') {
it('should generate middleware SSR manifests for Node.js', async () => {
Expand Down
Expand Up @@ -3,7 +3,12 @@ import webdriver from 'next-webdriver'
import cheerio from 'cheerio'
import { renderViaHTTP, check } from 'next-test-utils'

export default function (context) {
function getNodeBySelector(html, selector) {
const $ = cheerio.load(html)
return $(selector)
}

export default function (context, runtime) {
it('should render server components correctly', async () => {
const homeHTML = await renderViaHTTP(context.appPort, '/', null, {
headers: {
Expand All @@ -29,8 +34,10 @@ export default function (context) {

it('should support next/link in server components', async () => {
const linkHTML = await renderViaHTTP(context.appPort, '/next-api/link')
const $ = cheerio.load(linkHTML)
const linkText = $('div[hidden] > a[href="/"]').text()
const linkText = getNodeBySelector(
linkHTML,
'div[hidden] > a[href="/"]'
).text()

expect(linkText).toContain('go home')

Expand All @@ -52,25 +59,36 @@ export default function (context) {
expect(await browser.eval('window.beforeNav')).toBe(1)
})

it('should suspense next/image in server components', async () => {
const imageHTML = await renderViaHTTP(context.appPort, '/next-api/image')
const $ = cheerio.load(imageHTML)
const imageTag = $('div[hidden] > span > span > img')
// Disable next/image for nodejs runtime temporarily
if (runtime === 'edge') {
it('should suspense next/image in server components', async () => {
const imageHTML = await renderViaHTTP(context.appPort, '/next-api/image')
const imageTag = getNodeBySelector(
imageHTML,
'div[hidden] > span > span > img'
)

expect(imageTag.attr('src')).toContain('data:image')
})
expect(imageTag.attr('src')).toContain('data:image')
})
}

it('should handle multiple named exports correctly', async () => {
const clientExportsHTML = await renderViaHTTP(
context.appPort,
'/client-exports'
)
const $clientExports = cheerio.load(clientExportsHTML)
expect($clientExports('div[hidden] > div > #named-exports').text()).toBe(
'abcde'
)

expect(
getNodeBySelector(
clientExportsHTML,
'div[hidden] > div > #named-exports'
).text()
).toBe('abcde')
expect(
$clientExports('div[hidden] > div > #default-exports-arrow').text()
getNodeBySelector(
clientExportsHTML,
'div[hidden] > div > #default-exports-arrow'
).text()
).toBe('client-default-export-arrow')

const browser = await webdriver(context.appPort, '/client-exports')
Expand All @@ -83,4 +101,22 @@ export default function (context) {
expect(textNamedExports).toBe('abcde')
expect(textDefaultExportsArrow).toBe('client-default-export-arrow')
})

it('should handle 404 requests and missing routes correctly', async () => {
const id = '#text'
const content = 'custom-404-page'
const page404HTML = await renderViaHTTP(context.appPort, '/404')
const pageUnknownHTML = await renderViaHTTP(context.appPort, '/no.where')

const page404Browser = await webdriver(context.appPort, '/404')
const pageUnknownBrowser = await webdriver(context.appPort, '/no.where')

expect(await page404Browser.waitForElementByCss(id).text()).toBe(content)
expect(await pageUnknownBrowser.waitForElementByCss(id).text()).toBe(
content
)

expect(getNodeBySelector(page404HTML, id).text()).toBe(content)
expect(getNodeBySelector(pageUnknownHTML, id).text()).toBe(content)
})
}