rsnext/test
Damien Simonin Feugas bf089562c7
feat(middleware)!: forbids middleware response body (#36835)
_Hello Next.js team! First PR here, I hope I've followed the right practices._

### What's in there?

It has been decided to only support the following uses cases in Next.js' middleware:
- rewrite the URL (`x-middleware-rewrite` response header)
- redirect to another URL (`Location` response header)
- pass on to the next piece in the request pipeline (`x-middleware-next` response header)

1. during development, a warning on console tells developers when they are returning a response (either with `Response` or `NextResponse`).
2. at build time, this warning becomes an error.
3. at run time, returning a response body will trigger a 500 HTTP error with a JSON payload containing the detailed error.

All returned/thrown errors contain a link to the documentation.

This is a breaking feature compared to the _beta_ middleware implementation, and also removes `NextResponse.json()` which makes no sense any more.

### How to try it?
- runtime behavior: `HEADLESS=true yarn jest test/integration/middleware/core`
- build behavior : `yarn jest test/integration/middleware/build-errors`
- development behavior: `HEADLESS=true yarn jest test/development/middleware-warnings`

### Notes to reviewers

The limitation happens in next's web adapter. ~The initial implementation was to check `response.body` existence, but it turns out [`Response.redirect()`](https://github.com/vercel/next.js/blob/canary/packages/next/server/web/spec-compliant/response.ts#L42-L53) may set the response body (https://github.com/vercel/next.js/pull/31886). Hence why the proposed implementation specifically looks at response headers.~
`Response.redirect()` and `NextResponse.redirect()` do not need to include the final location in their body: it is handled by next server https://github.com/vercel/next.js/blob/canary/packages/next/server/next-server.ts#L1142

Because this is a breaking change, I had to adjust several tests cases, previously returning JSON/stream/text bodies. When relevant, these middlewares are returning data using response headers.

About DevEx: relying on AST analysis to detect forbidden use cases is not as good as running the code.
Such cases are easy to detect:
```js
new Response('a text value')
new Response(JSON.stringify({ /* whatever */ })
```
But these are false-positive cases:
```js
function returnNull() { return null }
new Response(returnNull())

function doesNothing() {}
new Response(doesNothing())
```
However, I see no good reasons to let users ship middleware such as the one above, hence why the build will fail, even if _technically speaking_, they are not setting the response body. 



## Feature

- [x] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [x] Integration tests added
- [x] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [x] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [x] Make sure the linting passes by running `yarn lint`
2022-05-19 22:02:20 +00:00
..
.stats-app remove commons chunk config (#34445) 2022-02-19 22:26:54 +00:00
__mocks__ Validate streaming writer chunk type in testing (#36200) 2022-04-18 16:24:06 +02:00
development feat(middleware)!: forbids middleware response body (#36835) 2022-05-19 22:02:20 +00:00
e2e feat(middleware)!: forbids middleware response body (#36835) 2022-05-19 22:02:20 +00:00
integration feat(middleware)!: forbids middleware response body (#36835) 2022-05-19 22:02:20 +00:00
lib fix(middleware): false positive dynamic code detection at build time (#36955) 2022-05-17 19:35:48 +00:00
production feat(middleware)!: forbids middleware response body (#36835) 2022-05-19 22:02:20 +00:00
unit feat(middleware)!: forbids middleware response body (#36835) 2022-05-19 22:02:20 +00:00
.gitignore Universal Webpack (#3578) 2018-01-30 16:44:44 +01:00
jest-setup-after-env.ts Update azure config (#33999) 2022-02-04 13:42:22 -06:00
readme.md Add handling for testing against deployments (#36285) 2022-04-20 12:23:09 +00:00
test-file.txt Add additional file serving tests (#12479) 2020-05-04 11:58:19 -05:00

Writing tests for Next.js

Getting Started

You can set-up a new test using yarn new-test which will start from a template related to the test type.

Test Types in Next.js

  • e2e: These tests will run against next dev, next start, and deployed to Vercel
  • development: These tests only run against next dev
  • production: These tests will run against next start.
  • integration: These tests run misc checks and modes and is where tests used to be added before we added more specific folders. Ideally we don't add new test suites here as tests here are not isolated from the monorepo.
  • unit: These are very fast tests that should run without a browser or running next and should be testing a specific utility.

For the e2e, production, and development tests the createNext utility should be used and an example is available here. This creates an isolated Next.js install to ensure nothing in the monorepo is relied on accidentally causing incorrect tests.

All new test suites should be written in TypeScript either .ts (or .tsx for unit tests). This will help ensure we catch smaller issues in tests that could cause flakey or incorrect tests.

If a test suite already exists that relates closely to the item being tested (e.g. hash navigation relates to existing navigation test suites) the new checks can be added in the existing test suite.

Best Practices

  • When checking for a condition that might take time, ensure it is waited for either using the browser waitForElement or using the check util in next-test-utils.
  • When applying a fix, ensure the test fails without the fix. This makes sure the test will properly catch regressions.