Skip to content

Commit

Permalink
Remove setLazyProp from the hot path of all requests (#55391)
Browse files Browse the repository at this point in the history
Removes setLazyProp from the hot path when handling requests. Noticed in
the CPU profile that `Object.defineProperty` is not ideal so this
ensures it's only called where it's needed, which is in `pages`
rendering and `pages/api`.

Also added a test for `getServerSideProps` reading cookies, which is
supported but a test was missing.

<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation

- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide

### Adding or Updating Examples

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md


## For Maintainers

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->
  • Loading branch information
timneutkens authored Sep 14, 2023
1 parent a94f5a5 commit 4385efa
Showing 10 changed files with 58 additions and 35 deletions.
21 changes: 21 additions & 0 deletions packages/next/src/server/api-utils/get-cookie-parser.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import type { NextApiRequestCookies } from '.'

/**
* Parse cookies from the `headers` of request
* @param req request object
*/

export function getCookieParser(headers: {
[key: string]: string | string[] | null | undefined
}): () => NextApiRequestCookies {
return function parseCookie(): NextApiRequestCookies {
const { cookie } = headers

if (!cookie) {
return {}
}

const { parse: parseCookieFn } = require('next/dist/compiled/cookie')
return parseCookieFn(Array.isArray(cookie) ? cookie.join('; ') : cookie)
}
}
19 changes: 0 additions & 19 deletions packages/next/src/server/api-utils/index.ts
Original file line number Diff line number Diff line change
@@ -18,25 +18,6 @@ export type __ApiPreviewProps = {
previewModeSigningKey: string
}

/**
* Parse cookies from the `headers` of request
* @param req request object
*/
export function getCookieParser(headers: {
[key: string]: string | string[] | null | undefined
}): () => NextApiRequestCookies {
return function parseCookie(): NextApiRequestCookies {
const { cookie } = headers

if (!cookie) {
return {}
}

const { parse: parseCookieFn } = require('next/dist/compiled/cookie')
return parseCookieFn(Array.isArray(cookie) ? cookie.join('; ') : cookie)
}
}

/**
*
* @param res response object
2 changes: 1 addition & 1 deletion packages/next/src/server/api-utils/node.ts
Original file line number Diff line number Diff line change
@@ -15,7 +15,6 @@ import isError from '../../lib/is-error'
import { isResSent } from '../../shared/lib/utils'
import { interopDefault } from '../../lib/interop-default'
import {
getCookieParser,
setLazyProp,
sendStatusCode,
redirect,
@@ -27,6 +26,7 @@ import {
SYMBOL_PREVIEW_DATA,
RESPONSE_LIMIT_DEFAULT,
} from './index'
import { getCookieParser } from './get-cookie-parser'
import { getTracer } from '../lib/trace/tracer'
import { NodeSpan } from '../lib/trace/constants'
import { RequestCookies } from '../web/spec-extension/cookies'
3 changes: 2 additions & 1 deletion packages/next/src/server/base-http/index.ts
Original file line number Diff line number Diff line change
@@ -2,7 +2,8 @@ import type { IncomingHttpHeaders, OutgoingHttpHeaders } from 'http'
import type { I18NConfig } from '../config-shared'

import { PERMANENT_REDIRECT_STATUS } from '../../shared/lib/constants'
import { getCookieParser, NextApiRequestCookies } from '../api-utils'
import { NextApiRequestCookies } from '../api-utils'
import { getCookieParser } from '../api-utils/get-cookie-parser'

export interface BaseNextRequestConfig {
basePath: string | undefined
8 changes: 1 addition & 7 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
@@ -50,11 +50,7 @@ import {
TEMPORARY_REDIRECT_STATUS,
} from '../shared/lib/constants'
import { isDynamicRoute } from '../shared/lib/router/utils'
import {
setLazyProp,
getCookieParser,
checkIsOnDemandRevalidate,
} from './api-utils'
import { checkIsOnDemandRevalidate } from './api-utils'
import { setConfig } from '../shared/lib/runtime-config.shared-runtime'

import { setRevalidateHeaders } from './send-payload/revalidate-headers'
@@ -791,8 +787,6 @@ export default abstract class Server<ServerOptions extends Options = Options> {
return
}

setLazyProp({ req: req as any }, 'cookies', getCookieParser(req.headers))

// Parse url if parsedUrl not provided
if (!parsedUrl || typeof parsedUrl !== 'object') {
parsedUrl = parseUrl(req.url!, true)
2 changes: 0 additions & 2 deletions packages/next/src/server/lib/router-utils/resolve-routes.ts
Original file line number Diff line number Diff line change
@@ -15,7 +15,6 @@ import { stringifyQuery } from '../../server-route-utils'
import { formatHostname } from '../format-hostname'
import { toNodeOutgoingHttpHeaders } from '../../web/utils'
import { isAbortError } from '../../pipe-readable'
import { getCookieParser, setLazyProp } from '../../api-utils'
import { getHostname } from '../../../shared/lib/get-hostname'
import { UnwrapPromise } from '../../../lib/coalesced-function'
import { getRedirectStatus } from '../../../lib/redirect-status'
@@ -154,7 +153,6 @@ export function getResolveRoutes(
addRequestMeta(req, '__NEXT_INIT_URL', initUrl)
addRequestMeta(req, '__NEXT_INIT_QUERY', { ...parsedUrl.query })
addRequestMeta(req, '_protocol', protocol)
setLazyProp({ req }, 'cookies', () => getCookieParser(req.headers)())

if (!isUpgradeReq) {
addRequestMeta(req, '__NEXT_CLONABLE_BODY', getCloneableBody(req))
10 changes: 9 additions & 1 deletion packages/next/src/server/render.tsx
Original file line number Diff line number Diff line change
@@ -15,7 +15,12 @@ import type {
} from '../shared/lib/utils'
import type { ImageConfigComplete } from '../shared/lib/image-config'
import type { Redirect } from '../lib/load-custom-routes'
import type { NextApiRequestCookies, __ApiPreviewProps } from './api-utils'
import {
type NextApiRequestCookies,
type __ApiPreviewProps,
setLazyProp,
} from './api-utils'
import { getCookieParser } from './api-utils/get-cookie-parser'
import type { FontManifest, FontConfig } from './font-utils'
import type { LoadComponentsReturnType, ManifestItem } from './load-components'
import type {
@@ -386,6 +391,9 @@ export async function renderToHTMLImpl(
renderOpts: Omit<RenderOpts, keyof RenderOptsExtra>,
extra: RenderOptsExtra
): Promise<RenderResult> {
// Adds support for reading `cookies` in `getServerSideProps` when SSR.
setLazyProp({ req: req as any }, 'cookies', getCookieParser(req.headers))

const renderResultMeta: RenderResultMetadata = {}

// In dev we invalidate the cache by appending a timestamp to the resource URL.
2 changes: 1 addition & 1 deletion packages/next/src/shared/lib/i18n/get-locale-redirect.ts
Original file line number Diff line number Diff line change
@@ -4,7 +4,7 @@ import { acceptLanguage } from '../../../server/accept-header'
import { denormalizePagePath } from '../page-path/denormalize-page-path'
import { detectDomainLocale } from './detect-domain-locale'
import { formatUrl } from '../router/utils/format-url'
import { getCookieParser } from '../../../server/api-utils'
import { getCookieParser } from '../../../server/api-utils/get-cookie-parser'

interface Options {
defaultLocale: string
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@ import {
isInterceptionRouteAppPath,
} from '../../../../server/future/helpers/interception-routes'
import { NEXT_RSC_UNION_QUERY } from '../../../../client/components/app-router-headers'
import { getCookieParser } from '../../../../server/api-utils/get-cookie-parser'

/**
* Ensure only a-zA-Z are used for param names for proper interpolating
@@ -64,7 +65,13 @@ export function matchHas(
break
}
case 'cookie': {
value = (req as any).cookies[hasItem.key]
if ('cookies' in req) {
value = req.cookies[hasItem.key]
} else {
const cookies = getCookieParser(req.headers)()
value = cookies[hasItem.key]
}

break
}
case 'query': {
17 changes: 15 additions & 2 deletions test/integration/typescript/test/index.test.js
Original file line number Diff line number Diff line change
@@ -21,8 +21,8 @@ const handleOutput = (msg) => {
output += msg
}

async function get$(path, query) {
const html = await renderViaHTTP(appPort, path, query)
async function get$(path, query, options) {
const html = await renderViaHTTP(appPort, path, query, options)
return cheerio.load(html)
}

@@ -49,6 +49,19 @@ describe('TypeScript Features', () => {
expect($('#cookies').text()).toBe('{}')
})

it('should render the cookies page with cookies', async () => {
const $ = await get$(
'/ssr/cookies',
{},
{
headers: {
Cookie: 'key=value;',
},
}
)
expect($('#cookies').text()).toBe(`{"key":"value"}`)
})

it('should render the generics page', async () => {
const $ = await get$('/generics')
expect($('#value').text()).toBe('Hello World from Generic')

0 comments on commit 4385efa

Please sign in to comment.