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

update handler to accept optional NextResponse for nextjs 13 app rout… #136

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

pengyanb
Copy link

  • Minor update to the handler signature to accept an optional NextResponse parameter.
  • Minor update to README.md file, to demonstrate how cookies can be added to the res object in the "graphql resolver".

@pengyanb pengyanb requested a review from a team as a code owner July 25, 2023 11:30
@trevor-scheer
Copy link
Member

@martinnabhan would you mind taking a look at this one?

@martinnabhan
Copy link
Collaborator

I'm not sure how I feel about passing a NextResponse to the context just for the sake of copying its headers (and cookies) onto the actual response? Might cause confusion when someone tries to use res for anything else, since res won't be the actual response object.

Instead I suggest we might add a headers property to the default context, which will have its contents copied onto the final response.

What does everyone think?

@luchillo17
Copy link

This PR has been stuck for over 6 months, and is outdated against the latest version and all, @pengyanb do you think you can check the latest version and see if this PR is still valid?

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.

None yet

4 participants