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

fix: delete request body from server adapter #30

Merged
merged 7 commits into from
Feb 1, 2023

Conversation

khuezy
Copy link
Collaborator

@khuezy khuezy commented Jan 15, 2023

This PR fixes some issues:

  1. Nextjs doesn't parse the body if it already exists on the request object: https://github.com/vercel/next.js/blob/canary/packages/next/src/server/api-utils/node.ts#L474

  2. If a header has a space it in, the custom header ends up w/ null fields

  3. compress: false has been fixed in the latest nextjs version.

EDIT: Do not merge yet

@changeset-bot
Copy link

changeset-bot bot commented Jan 15, 2023

⚠️ No Changeset found

Latest commit: b336f1c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jan 15, 2023

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

Name Status Preview Comments Updated
open-next ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 1, 2023 at 10:27PM (UTC)

@timothymiller
Copy link

Hey khuezy,

This issue is affecting me as well. I would love to use next-auth with open-next.

How can I help?

@timothymiller
Copy link

timothymiller commented Jan 20, 2023

I would like to add I am getting a 'crypto is not defined' error when using next-auth middleware to provide per page authentication.

I am deploying with node 18.13.0, next-auth 4.18.6, sst 2.0.0-rc.41, and next 13.1.3

My middleware.ts code looks like this:

export { default } from "next-auth/middleware" // require authentication

export const config = { matcher: ["/:path*"] } // all pages

Screenshot 2023-01-20 at 17 39 41

@khuezy
Copy link
Collaborator Author

khuezy commented Jan 20, 2023

Another person on another ticket was experiencing the same crypto error as you... crypto comes w/ nodejs so that error is confusing.

@timothymiller
Copy link

If memory serves me right, middleware functions run in a slimmed down version of node called 'edge runtime'

https://nextjs.org/docs/api-reference/edge-runtime

Could be relevant

@khuezy
Copy link
Collaborator Author

khuezy commented Jan 20, 2023

As long as your api/ functions aren't exporting the config.runtime = 'edge', it shouldn't be using it.

@khuezy
Copy link
Collaborator Author

khuezy commented Jan 20, 2023

Are you doing pnpx sst deploy?

@timothymiller
Copy link

Yes. I am doing pnpx sst deploy

I am not exporting config.runtime = 'edge' either

@khuezy
Copy link
Collaborator Author

khuezy commented Jan 23, 2023

Do npx sst deploy instead. I don't know why pnpx is causing issues.

@timothymiller
Copy link

Doing that now. @khuezy That should fix the crypto issue?

@khuezy
Copy link
Collaborator Author

khuezy commented Jan 23, 2023

Another person had the same issue and resolved it using npx

@khuezy
Copy link
Collaborator Author

khuezy commented Jan 31, 2023

@timothymiller
Copy link

I tried running the next-auth checks in getServerSideProps in place of middleware...

My code works on Vercel, but not with SST. The client does not redirect to Cognito as expected.

Can provide a code example and versions if necessary.

@khuezy
Copy link
Collaborator Author

khuezy commented Jan 31, 2023

Did you monkeypatch the changes in this PR before deploying?

@fwang
Copy link
Contributor

fwang commented Feb 1, 2023

Merging into master.

I reverted the compress setting as CloudFront should already compressed the response. Let's open a separate PR if it is still required.

@fwang fwang merged commit b4e096b into sst:main Feb 1, 2023
@revmischa
Copy link
Contributor

You don't know what I went through tracking this problem down in cdk-nextjs lol

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