Throw error if res is accessed after gSSP returns

[kara]

Currently it's possible to access the ServerResponse through context.res
in getServerSideProps(). If one was to store that response and mutate
its headers or status code after gSSP returns (e.g. during rendering), it
would currently happen to work because of when headers are sent. However,
this is an anti-pattern that relies an implementation detail of the framework
and shouldn't be allowed. This will be particularly important once Next.js
starts to support basic streaming (two-part flush: routing then data) because
then the headers will be sent as soon as gSSP returns, which explicitly breaks
this pattern.

With this commit, the framework now throws an error in development mode if
the ServerResponse is accessed after gSSP returns.

Bug

• Related issues linked using fixes #number
• Integration tests added
• Errors have helpful link attached, see contributing.md

[kara]

Note: this PR takes on a more aggressive approach of throwing for all dev mode accesses of res after gSSP returns, which could be considered breaking. There is a more conservative option to only throw if props is a promise (i.e. using the new feature) and to simply warn for existing cases. If preferred, happy to refactor to take that approach too

[ijjk]

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	kara/next.js res	Change
buildDuration	14.5s	14.7s	⚠️ +211ms
buildDurationCached	3.7s	3.7s	-38ms
nodeModulesSize	47 MB	47 MB	⚠️ +1.14 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	kara/next.js res	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.067	3.052	-0.02
/ avg req/sec	815.07	819.19	+4.12
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.493	1.502	⚠️ +0.01
/error-in-render avg req/sec	1674.37	1664.85	⚠️ -9.52

Client Bundles (main, webpack, commons)

	vercel/next.js canary	kara/next.js res	Change
745.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	26.6 kB	26.6 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	70.4 kB	70.4 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	kara/next.js res	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	kara/next.js res	Change
_app-HASH.js gzip	979 B	979 B	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	329 B	329 B	✓
dynamic-HASH.js gzip	2.67 kB	2.67 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	918 B	918 B	✓
image-HASH.js gzip	4.14 kB	4.14 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	318 B	318 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	13 kB	13 kB	✓

Client Build Manifests

	vercel/next.js canary	kara/next.js res	Change
_buildManifest.js gzip	492 B	492 B	✓
Overall change	492 B	492 B	✓

Rendered Page Sizes

	vercel/next.js canary	kara/next.js res	Change
index.html gzip	539 B	539 B	✓
link.html gzip	552 B	552 B	✓
withRouter.html gzip	533 B	533 B	✓
Overall change	1.62 kB	1.62 kB	✓

---

Webpack 4 Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	kara/next.js res	Change
buildDuration	11.9s	11.9s	-30ms
buildDurationCached	5s	4.9s	-81ms
nodeModulesSize	47 MB	47 MB	⚠️ +1.14 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	kara/next.js res	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.086	3.171	⚠️ +0.08
/ avg req/sec	810.01	788.32	⚠️ -21.69
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.526	1.463	-0.06
/error-in-render avg req/sec	1638.37	1709.21	+70.84

Client Bundles (main, webpack, commons)

	vercel/next.js canary	kara/next.js res	Change
16.HASH.js gzip	186 B	186 B	✓
677f882d2ed8..HASH.js gzip	14.1 kB	14.1 kB	✓
framework.HASH.js gzip	41.9 kB	41.9 kB	✓
main-HASH.js gzip	13.8 kB	13.8 kB	✓
webpack-HASH.js gzip	1.19 kB	1.19 kB	✓
Overall change	71.2 kB	71.2 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	kara/next.js res	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	kara/next.js res	Change
_app-HASH.js gzip	964 B	964 B	✓
_error-HASH.js gzip	3.8 kB	3.8 kB	✓
amp-HASH.js gzip	552 B	552 B	✓
css-HASH.js gzip	333 B	333 B	✓
dynamic-HASH.js gzip	2.87 kB	2.87 kB	✓
head-HASH.js gzip	3.06 kB	3.06 kB	✓
hooks-HASH.js gzip	924 B	924 B	✓
index-HASH.js gzip	231 B	231 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	298 B	298 B	✓
script-HASH.js gzip	3.03 kB	3.03 kB	✓
withRouter-HASH.js gzip	295 B	295 B	✓
30809af5c834..565.css gzip	125 B	125 B	✓
Overall change	18.1 kB	18.1 kB	✓

Client Build Manifests

	vercel/next.js canary	kara/next.js res	Change
_buildManifest.js gzip	500 B	500 B	✓
Overall change	500 B	500 B	✓

Rendered Page Sizes

	vercel/next.js canary	kara/next.js res	Change
index.html gzip	585 B	585 B	✓
link.html gzip	597 B	597 B	✓
withRouter.html gzip	578 B	578 B	✓
Overall change	1.76 kB	1.76 kB	✓

Commit: aabd309

[ijjk]

Failing test suites

Commit: aabd309

test/development/acceptance/ReactRefreshLogBoxMisc.test.ts

• ReactRefreshLogBox > with multiple children
• ReactRefreshLogBox > server-side only compilation errors

Expand output

● ReactRefreshLogBox › with multiple children

expect(received).toMatchInlineSnapshot(snapshot)

Snapshot name: `ReactRefreshLogBox <Link> with multiple children 1`

- Snapshot  - 1
+ Received  + 2

- Error: Multiple children were passed to <Link> with `href` of `/` but only one child is supported https://nextjs.org/docs/messages/link-multiple-children
+ Error: Multiple children were passed to <Link> with `href` of `/` but only one child is supported https://nextjs.org/docs/messages/link-multiple-children 
+ Open your browser's console to view the Component stack trace.

  82 |       `
  83 |     )
> 84 |     expect(await session.hasRedbox()).toBe(false)
     |                                                 ^
  85 |
  86 |     await session.patch(
  87 |       'index.js',

  at Object.<anonymous> (development/acceptance/ReactRefreshLogBoxMisc.test.ts:84:49)

● ReactRefreshLogBox › server-side only compilation errors

expect(received).toBe(expected) // Object.is equality

Expected: true
Received: false

  171 |               passHref={undefined}
  172 |               prefetch={undefined}
> 173 |             >
      |              ^
  174 |               Abc
  175 |             </Link>
  176 |           )

  at Object.<anonymous> (development/acceptance/ReactRefreshLogBoxMisc.test.ts:173:47)

[kara]

Note: Looks like failing test suite is already broken on canary: https://github.com/vercel/next.js/runs/3591780616

[poacher2k]

I'm getting this error on 11.1.3-canary.69 when using next-connect with express-session:

export async function getServerSideProps(ctx: NextPageContext) {
	if (!ctx.req || !ctx.res) {
		return { props: {} };
	}

	const handler = nextConnect();
	handler.use(sessionMiddleware);
	await handler.run(ctx.req, ctx.res);

	return {
		props: {},
	};
}

Does this change mean that using express-session in this way will no longer be supported?

[poacher2k]

I'm guessing this is caused by express-session proxying end() to commit the session? https://github.com/expressjs/session/blob/a8641429502fcc076c4b2dcbd6b2320891c1650c/index.js#L246

[poacher2k]

I'm guessing that since next-session uses a similar approach to express-session to saving the session, is the reason that the basic next-session getServiceSideProps example also fails with this same error.

[devknoll]

@poacher2k The hooks that these libraries are doing only really make sense when there is another middleware that is also using the session. In a Next.js app, that will only happen if you’re using a custom server.

So if you’re using a custom server, I would recommend installing the session middleware there, rather than in gSSP.

If you’re not using a custom server, then you don’t actually need the hooks. With next-session, it looks like there is an option to disable the hooks. Instead, you would just decide whether you need to commit the header before returning from getServerSideProps. I would lobby express-session for a similar option if necessary (or similar, e.g. NOOPing the hooks if the session is manually saved or destroyed, or reverting them yourself in a pinch).

That said, this probably shouldn’t break your app, only prevent you from adopting new features. @kara it might make sense to make this a warning instead.