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(ts): add overloads to withAuth middleware #7999

Merged
merged 5 commits into from
Jul 12, 2023

Conversation

rexfordessilfie
Copy link
Contributor

@rexfordessilfie rexfordessilfie commented Jul 10, 2023

☕️ Reasoning

The withAuth middleware returns different values depending on provided arguments. In the current implementation it returns a union such as:NextMiddlewareWithAuth | NextMiddlewareResult | ....

This union is type is not narrow enough for use in scenarios where we expect a callable NextMiddlewareWithAuth that we can invoke later after getting some outside context from the middleware request object.

In summary, when we want to do something like this:

export default middleware(req: Request, event: NextFetchEvent){
   // ...do something with Request
   const response = withAuth({...})(req, event)
   // ...do something with Response?
  return response
}

// Side note: This PR also resolves type issues that would arise from doing this instead 
// (for the reason that Request does not extend `NextRequestWithAuth`:
// const response = withAuth(req, {...})

Solution: This PR adds type overloads such that the types narrow down as expected depending on the arguments supplied to withAuth. This lets us do the above without any type issues.

🧢 Checklist

  • Documentation (N/A)
  • Tests (N/A)
  • Ready to be merged

🎫 Affected issues

Please scout and link issues that might be solved by this PR.

Fixes: #7997

📌 Resources

Sorry, something went wrong.

@vercel
Copy link

vercel bot commented Jul 10, 2023

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

Name Status Preview Comments Updated (UTC)
next-auth-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 12, 2023 3:36am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
auth-docs ⬜️ Ignored (Inspect) Visit Preview Jul 12, 2023 3:36am

@vercel
Copy link

vercel bot commented Jul 10, 2023

@rexfordessilfie is attempting to deploy a commit to the authjs Team on Vercel.

A member of the Team first needs to authorize it.

@balazsorban44
Copy link
Member

Thanks, could you open this against the v4 branch? 🙏

@rexfordessilfie
Copy link
Contributor Author

Thanks! Will do.

@rexfordessilfie
Copy link
Contributor Author

Done, @balazsorban44 ✅. Let me know if I did everything right!

Copy link
Member

@ThangHuuVu ThangHuuVu left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed bug report and the PR, @rexfordessilfie 🙏 I left a comment, let me know what you think!

Comment on lines 216 to 219
): <Req extends Request = NextRequestWithAuth>(
request: Req,
event: NextFetchEvent
) => ReturnType<NextMiddlewareWithAuth>
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this type is NextMiddlewareWithAuth with a different request type
How do you think if we do it like this:

export function withAuth(): ReturnType<NextMiddlewareWithAuth>

export function withAuth(
  req: NextRequestWithAuth
): ReturnType<NextMiddlewareWithAuth>

export function withAuth(
  req: NextRequestWithAuth,
  event: NextFetchEvent
): ReturnType<NextMiddlewareWithAuth>

export function withAuth(
  req: NextRequestWithAuth,
  options: NextAuthMiddlewareOptions
): ReturnType<NextMiddlewareWithAuth>

export function withAuth(
  middleware: NextMiddlewareWithAuth,
  options: NextAuthMiddlewareOptions
): NextMiddlewareWithAuth

export function withAuth(
  middleware: NextMiddlewareWithAuth
): NextMiddlewareWithAuth

export function withAuth(
  options: NextAuthMiddlewareOptions
): NextMiddlewareWithAuth

Doing this way, we will keep the params consistent with the return type: ReturnType<NextMiddlewareWithAuth> | NextMiddlewareWithAuth.

In the app code, it will be like this:

import { NextRequestWithAuth, withAuth } from "next-auth/middleware"
import { NextFetchEvent } from "next/server";

export default async function middleware(req: NextRequestWithAuth, event: NextFetchEvent) {
  const localeStr = req.headers.get("Accept-Language");
  const locale = localeStr?.split(";")[0].split(",")[0];

  const withAuthMiddleware = withAuth({
    callbacks: {},
    pages: {
      signIn: `/?locale=${locale}`,
    },
  });

  return await withAuthMiddleware(req, event);
}

One different is how we type the req object - we can use NextRequestWithAuth for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! Yes, you are right. It was a copy directly from NextMiddlewareWithAuth.

I did not consider using req: NextRequestWithAuth directly. I noticed I was getting type errors when it was req: Request and so I made a generic version of NextMiddlewareWithAuth to fix the type errors!

I will switch to this and request a re-review!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@ThangHuuVu ThangHuuVu left a comment

Choose a reason for hiding this comment

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

LGTM!

@masterkain
Copy link

#13 38.29 Failed to compile.
#13 38.29 
#13 38.29 ./app/api/auth/[...nextauth]/route.ts:4:17
#13 38.29 Type error: This expression is not callable.
#13 38.29   Type 'typeof import("/app/node_modules/next-auth/next")' has no call signatures.
#13 38.29 
#13 38.29   2 | import NextAuth from 'next-auth';
#13 38.29   3 | 
#13 38.29 > 4 | const handler = NextAuth(authOptions);
#13 38.29     |                 ^
#13 38.29   5 | 
#13 38.29   6 | export { handler as GET, handler as POST };
#13 38.29   7 | 
#13 ERROR: process "/bin/sh -c yarn build" did not complete successfully: exit code: 1

my build is failing, app/api/auth/[...nextauth]/route.ts:

import { authOptions } from '@/lib/auth';
import NextAuth from 'next-auth';

const handler = NextAuth(authOptions);

export { handler as GET, handler as POST };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy Refers to `next-auth` v4. Minimal maintenance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

withAuth return types do not narrow down to middleware type
4 participants