Cannot use type on app router API route handler `request: Request`

michaeloliverx commented 1 year ago

Checklist

[X] The issue can be reproduced in the nextjs-auth0 sample app (or N/A).
[X] I have looked into the Readme, Examples, and FAQ and have not found a suitable solution or answer.
[X] I have looked into the API documentation and have not found a suitable solution or answer.
[X] I have searched the issues and have not found a suitable solution or answer.
[X] I have searched the Auth0 Community forums and have not found a suitable solution or answer.
[X] I agree to the terms within the Auth0 Code of Conduct.

Description

Next.js 13 supports using Request and Response types, Auth0 SDK complains about types.

Reproduction

import { withApiAuthRequired, getAccessToken } from "@auth0/nextjs-auth0/edge";

const GET = withApiAuthRequired(async (request: Request) => {
  const { accessToken } = await getAccessToken({refresh: true});

  return Response.json({ accessToken });
});

export { GET };

export const runtime = "edge";

Additional context

No response

nextjs-auth0 version

3.1.0

Next.js version

13.5.3

Node.js version

16.19.1

adamjmcgrath commented 1 year ago

Hi @michaeloliverx - thanks for raising this

The request object is a NextRequest (see https://nextjs.org/docs/app/api-reference/file-conventions/route#parameters) (The SDK relies on this because it uses the cookies helper of NextRequest)

What's the problem with you using NextRequest as the type?

michaeloliverx commented 1 year ago

Thanks for the fast response!

The SDK relies on this because it uses the cookies helper of NextRequest)

I wonder if the SDK used the cookies function would it mean user code could use either type?

import { cookies } from 'next/headers'

...

const cookieStore = cookies()

What's the problem with you using NextRequest as the type?

Nothing really I just wasn't aware what the problem was and usually I prefer to use the native Request and Response globals.

I guess a note in the docs might help others as well?

adamjmcgrath commented 1 year ago

I wonder if the SDK used the cookies function would it mean user code could use either type?

Even if the SDK didn't use req.cookies, the Next.js route accepts a NextRequest whether you type it as a Request or not.

I guess a note in the docs might help others as well?

I don't think a special callout is required for this SDK just accurately typing a Next api route

michaeloliverx commented 1 year ago

Even if the SDK didn't use req.cookies, the Next.js route accepts a NextRequest whether you type it as a Request or not.

I totally get what you are saying and using NextRequest would be more "correct" but the Next.js docs show examples using the Request type and returning a new Response instance.

The cookies and headers docs show examples for both Request and NextRequest but the Request Body example only shows using Request. In fact the docs are littered with examples that only refer to the native fetch primitives Request/Response. I think this can be confusing for users of this SDK (it was for me).

Also if you look at the streaming section on the same page, that example returns a StreamingTextResponse instance to stream LLM model responses. That class is typed as a extension of the native Response object:

/**
 * A utility class for streaming text responses.
 */
declare class StreamingTextResponse extends Response {
    constructor(res: ReadableStream, init?: ResponseInit, data?: experimental_StreamData);
}

I am personally using the Vercel AI SDK (https://github.com/vercel/ai) so after changing the request type to NextRequest I still get a type error

import { withApiAuthRequired } from "@auth0/nextjs-auth0/edge";
import OpenAI from "openai";
import { type NextRequest } from "next/server";
import { OpenAIStream, StreamingTextResponse } from "ai";

export const runtime = "edge";

const openai = new OpenAI({
  apiKey: process.env.OPENAI_API_KEY,
});

export const GET = withApiAuthRequired(async (request: NextRequest) => {
  const { messages } = await request.json();

  const response = await openai.chat.completions.create({
    model: "gpt-4-0613",
    stream: true,
    messages: [...messages],
  });

  const stream = OpenAIStream(response, {});

  return new StreamingTextResponse(stream, {});
});

TS output:

Overload 1 of 2, '(apiRoute: AppRouteHandlerFn): AppRouteHandlerFn', gave the following error.
    Argument of type '(request: NextRequest) => Promise<StreamingTextResponse>' is not assignable to parameter of type 'AppRouteHandlerFn'.
      Type 'Promise<StreamingTextResponse>' is not assignable to type 'NextResponse<unknown> | Promise<NextResponse<unknown>>'.
        Type 'Promise<StreamingTextResponse>' is not assignable to type 'Promise<NextResponse<unknown>>'.
          Type 'StreamingTextResponse' is missing the following properties from type 'NextResponse<unknown>': cookies, [INTERNALS]

From a user and DX perspective it would be a great win if it just worked with the Request and Response.

adamjmcgrath commented 1 year ago

I still think NextRequest is the right type for the request object, but I've just noticed your comment about the type of the response

Also if you look at the streaming section on the same page, that example returns a StreamingTextResponse instance to stream LLM model responses.

You are correct, the function you pass to withApiAuthRequired can return a Response (or any superset thereof) - so the return type should be Response, not NextResponse - will change that.

I am personally using the Vercel AI SDK (vercel/ai) so after changing the request type to NextRequest I still get a type error

Can you confirm if your code snippet you've shared actually works? (if you ignore the type error) I haven't tested this with a StreamingTextResponse, and I'm not sure if the way I'm cloning a Request will suffice for this use case

michaeloliverx commented 1 year ago

Can you confirm if your code snippet you've shared actually works? (if you ignore the type error) I haven't tested this with a StreamingTextResponse, and I'm not sure if the way I'm cloning a Request will suffice for this use case

The snippet I shared isn't the exact code I am running as I stripped out all the logic.. BUT it does seem to work yes!

Hitting it without auth gives me a 401 with

{
    "error": "not_authenticated",
    "description": "The user does not have an active session or is not authenticated",
}

And hitting it with does stream the text fine.

adamjmcgrath commented 1 year ago

Great - thanks for confirming @michaeloliverx

Will follow up with a PR shortly

auth0 / nextjs-auth0