upstash / qstash-js

Message queue for serverless
https://docs.upstash.com/qstash
MIT License
158 stars 15 forks source link

no clone and cast but proper NextRequest() instantiation #94

Closed logemann closed 4 months ago

logemann commented 4 months ago

otherwise we lose properties like "nextUrl" or "searchPararms" what people want to use in their handlers.

Example: Triggering this cron URL: http://myservice.com/api/foo?data_size=1

And having this handler in NextJS:

async function handler(_req: NextRequest) {
  console.log(_req.url);
  console.log(_req.nextUrl);
  console.log(_req.nextUrl.searchParams);
  return ...
}

export const POST = verifySignatureAppRouter(handler);
export const GET = verifyGetInvocation(handler);

Will result in the following log:

-> http://myservice.com/api/foo?data_size=1 -> undefined -> undefined

So basically the user is downgraded to Standard Request object but getting it back as a NextRequest in his handler. Pretty misleading.

logemann commented 4 months ago

Hmmm. Cant be merged as is... Primary problem solved but now i get this: ⨯ TypeError: Body is unusable

logemann commented 4 months ago

When using const reqClone = new NextRequest(req.url);

instead of: const reqClone = new NextRequest(req);

it seems to work but i dont know if there are other side effect like missing headers/body in the newly created request.

ogzhanolguncu commented 4 months ago

@logemann I have to test this thoroughly, then merge it if its good

logemann commented 4 months ago

tested and can confirm that when using const reqClone = new NextRequest(req.url), we will lose the body, which of course is a non starter.

When doing: new NextRequest(req), i get: ⨯ TypeError: Body is unusable

So, i am cluelesss right now.

Update: "Body is unusable" is because when we wrap the original request in NextRequest, we can only read the body once because its not a clone and the handler cant read it then, because it was already read in the verifier via: await req.text()

Update2: Ok. Got the solution. But now i need to check how i can do another commit on this PR.

logemann commented 4 months ago

This last commit should do it....

ogzhanolguncu commented 4 months ago

This last commit should do it....

I'll test it and let you know.

ogzhanolguncu commented 4 months ago

If you try to new NextRequest(request) when passing to handler. It throws like this:

TypeError: Cannot construct a Request with a Request object that has already been used.

Since we don't mutate the request object I don't think we need recreate it or clone it. We can directly pass it to handler function.

I test like this -> https://qstash-test.vercel.app/api/hello-world-1?foo=bar As you can see down below, now you can safely access those fields. image

I updated your PR accordingly.

logemann commented 4 months ago

can you ping me when the version is in NPM to try it out?

ogzhanolguncu commented 4 months ago

@logemann You can start using it at version 2.5.2.

jvitormelo commented 4 months ago

Humm, for some reason, after this version, I cannot use the verifySignatureAppRouter, I had to downgrade to 2.5.0 image

ogzhanolguncu commented 4 months ago

Can you send me a repo link for reproduction?

jvitormelo commented 4 months ago

Can you send me a repo link for reproduction?

Sorry for the late response, I checked and some person already did https://github.com/upstash/qstash-js/issues/100