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

feat(middleware)!: forbid response body #1

Closed
wants to merge 14 commits into from

Conversation

feugy
Copy link
Owner

@feugy feugy commented May 4, 2022

What's in there?

As part of the Edge Functions General Availability roadmap, 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.
It also removes NextResponse.json().

How to try it?

The new error is covered with:

  • integration tests:
    • runtime behavior: HEADLESS=true yarn jest test/integration/middleware/core
    • build behavior : HEADLESS=true 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() may return an empty stream. Hence why the proposed implementation specifically looks at response headers.

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:

new Response('a text value')
new Response(JSON.stringify({ /* whatever */ })

But these are false-positive cases:

function returnNull() { return null }
new Response(returnNull())

function doesNothing() {}
new Response(doesNothing())

However, I see no good reasons to let user build an middleware with the cases above, hence why the build will fail, even if technically speaking, they are not setting the response body.

@feugy feugy requested a review from javivelasco May 4, 2022 19:22
@feugy feugy force-pushed the feat/forbid-middleware-response-body branch from cfb312c to cf3ac85 Compare May 10, 2022 12:46
@feugy feugy marked this pull request as ready for review May 10, 2022 13:02
ijjk and others added 14 commits May 10, 2022 18:57
This aligns the output with what npm outputs. Without this change,
Next.js causes unwanted changes in package-lock.json.

## Bug

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

## Feature

- [ ] 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`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [x] Make sure the linting passes by running `yarn lint`
## Documentation / Examples

- Updating Tina Demo URL
- Adding Tina to blog-starter example
- Adding Tina to Basic features section
- Updating Tina to actually work again
* move FlightManifestPlugin to server compilers

* revert loader condition

* fix module id

* fix test and refactor

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* Update moduleNameMapper jest config and remove extra deps

* move svg mock to its own entry for easier overriding

* remove package script
When getInitialProps is customized with react 18, since gIP requires to return `html` as doc property which could be used by  user-land customization, we do blocking-rendering there and passdown the `html` to document

Fixes vercel#36675
Closes vercel#36419
Leverages Webpack builtin errors for better reporting
@feugy feugy force-pushed the feat/forbid-middleware-response-body branch from cf3ac85 to 26d4b06 Compare May 11, 2022 13:47
@feugy
Copy link
Owner Author

feugy commented May 11, 2022

Closing in favor of vercel#36835

@feugy feugy closed this May 11, 2022
feugy pushed a commit that referenced this pull request Dec 9, 2022
Enables `appDir` test for `deploy` in the `test/e2e/app-dir/index.test.ts` test suite.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants