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

Remove Edge Functions example that writes to the response body #261

Closed
wants to merge 4 commits into from

Conversation

SukkaW
Copy link

@SukkaW SukkaW commented May 20, 2022

Description

Ref: vercel/next.js#36835

Since Next.js v12.1.7-canary.10, Next.js page middleware removes the support of modifying the response body, and here I quote:

  1. during development, a warning on the 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.

And here are reasons why those examples should be removed:

edge-functions/cors

The example uses new Response(null, { statusCode: 204 }) to respond a CORS preflight request (An OPTIONS request) inside a "page middleware" which is now unsupported.

edge-functions/json-response and edge-functions/crypto

In these two examples' middlewares, they write a JSON.stringify-ed to a returned response body inside the "page middleware" which is now unsupported.

edge-functions/image-response

In this example's middleware, it writes a blob (containing an image binary) to a returned response body inside a "page middleware" which is now unsupported.


Also, there are still two examples that write to the response body in the middleware, which I will change in the coming PR:

edge-functions/basic-auth-password

In this example's middleware, it writes Auth required to the returned 401 response. I will change it to NextResponse.rewrite('/auth-required') in the coming PR.

edge-functions/geo-location-country-block

I will change it to NextResponse.rewrite in the coming PR.


Also cc @ijjk @feugy

Best Way to Test

N/A

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New Example
  • New feature in existing example

New Example Checklist

  • 🚀 Link to deployment URL on Vercel
  • 📱 Is it responsive? Are mobile and tablets considered?
  • 📚 README.md preview link in PR description (branch)
  • ⚙️ Secrets have instructions for how to set up in the readme
  • 🛫 npm run new-example was run to create the example

@vercel
Copy link

vercel bot commented May 20, 2022

@SukkaW is attempting to deploy a commit to the Vercel Solutions Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented May 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
edge-user-agent-based-rendering 🔄 Building (Inspect) May 20, 2022 at 6:40AM (UTC)
solutions-image-fallback 🔄 Building (Inspect) May 20, 2022 at 6:40AM (UTC)
solutions-script-component-ad 🔄 Building (Inspect) May 20, 2022 at 6:40AM (UTC)
solutions-script-component-strategies 🔄 Building (Inspect) May 20, 2022 at 6:40AM (UTC)
10 Ignored Deployments
Name Status Preview Updated
edge-functions-cookies ⬜️ Ignored (Inspect) May 20, 2022 at 6:40AM (UTC)
edge-functions-news ⬜️ Ignored (Inspect) May 20, 2022 at 6:40AM (UTC)
edge-maintenance-page ⬜️ Ignored (Inspect) May 20, 2022 at 6:40AM (UTC)
example-auth-with-ory ⬜️ Ignored (Inspect) May 20, 2022 at 6:40AM (UTC)
example-reduce-image-bandwidth-usage ⬜️ Ignored (Inspect) May 20, 2022 at 6:40AM (UTC)
solutions-combining-data-fetching-strategies ⬜️ Ignored (Inspect) May 20, 2022 at 6:40AM (UTC)
solutions-image-offset ⬜️ Ignored (Inspect) May 20, 2022 at 6:40AM (UTC)
solutions-loading-web-fonts ⬜️ Ignored (Inspect) May 20, 2022 at 6:40AM (UTC)
solutions-pagination-with-ssg ⬜️ Ignored (Inspect) May 20, 2022 at 6:40AM (UTC)
solutions-reuse-responses ⬜️ Ignored (Inspect) May 20, 2022 at 6:40AM (UTC)

@goncy
Copy link
Contributor

goncy commented May 20, 2022

Hi @SukkaW, thanks for realizing that this weren't relevant anymore with the last update, its really useful! I will talk with the team to see if we can keep any of them doing changes like a redirect or a rewrite but most seem safe to delete.

Will let you know soon and thanks again 🙌

@feugy
Copy link
Member

feugy commented May 20, 2022

Indeed, thanks for the hard work @SukkaW!

@SukkaW
Copy link
Author

SukkaW commented May 20, 2022

Hi @SukkaW, thanks for realizing that this weren't relevant anymore with the last update, its really useful! I will talk with the team to see if we can keep any of them doing changes like a redirect or a rewrite but most seem safe to delete.

Will let you know soon and thanks again 🙌

@goncy Thanks for your review!

IMHO these examples shouldn't exist in the first place: those are pages' middleware (and pages should be rendered in the browser) and it doesn't make sense to let a page's middleware return a JSON.

However, the API route of Next.js should be able to switch to the edge function runtime instead. I will raise a Feature Request toward Next.js.

Copy link
Member

@lfades lfades left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SukkaW Hi there and thank you for the PR 👋

We don't want to remove any examples yet, instead we want to update them to use redirects/rewrites, even if that doesn't make a lot of sense, for the case of crypto, image-response, and json-response` lets update the readme and link to the new API where we take the decision and reasoning behind it. The examples have been existing for a while now and removing them breaks shared links.

@SukkaW
Copy link
Author

SukkaW commented May 21, 2022

@SukkaW Hi there and thank you for the PR 👋

We don't want to remove any examples yet, instead we want to update them to use redirects/rewrites, even if that doesn't make a lot of sense, for the case of crypto, image-response, and json-response` lets update the readme and link to the new API where we take the decision and reasoning behind it. The examples have been existing for a while now and removing them breaks shared links.

Sounds fair.
I also notice that Next.js is adding Edge Runtime support for the API route (vercel/next.js#36947). I will change the example to use API route + Edge Runtime when the feature landed.

@lfades
Copy link
Member

lfades commented Jun 16, 2022

Closing this PR in favor of multiple PRs the team has been making to update the examples in a different way, thank you for the work you did here!

@lfades lfades closed this Jun 16, 2022
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.

4 participants