quirrel-dev / quirrel

The Task Queueing Solution for Serverless.
https://quirrel.dev
MIT License
885 stars 67 forks source link

added nextjs 13 app router native support & docs improvements. #1141

Closed KevinEdry closed 1 year ago

KevinEdry commented 1 year ago

Summary

This PR aims to revise and add two main things:

Nextjs 13 App Router

Because Nextjs 13 now expects a named method function as a route handler (POST, GET etc..), we need to provide it as part of the Queue and CronJob methods, we also want the queue instances themselves, so I've made it so it would be accessible via object destructuring, that way we can export both POST and the queue instance in one go, and alias the queue instance with a meaningful name.

Queue Usage:

  export const { POST, queue: sampleQueue } = Queue(
   "/api/queues/sample",
   (payload) => {
       ...logic goes here
  });

 await sampleQueue.enqueue({ jobData });

CronJob Usage:

  export const { POST } = CronJob(
   "/api/queues/sample",
   "0 0 * * 1"
   (payload) => {
       ...logic goes here
  });

I've added some docs changes as well to explain the usage of the new queue api.

The object destructuring is the best way I could think of to keep the named export function POST and still have a meaningful name for the queue instance, if you can think of a better implementation for this please let me know!

Changelog:

code

docs

BEGIN_COMMIT_OVERRIDE feat: added nextjs 13 app router native support & docs improvements END_COMMIT_OVERRIDE

netlify[bot] commented 1 year ago

Deploy Preview for quirrel-docs canceled.

Name Link
Latest commit f46f66ea4e319975578d70e465f50ceaff0ba392
Latest deploy log https://app.netlify.com/sites/quirrel-docs/deploys/6491c0282aad4300086b488c
netlify[bot] commented 1 year ago

Deploy request for quirrel-development-ui pending review.

Visit the deploys page to approve it

Name Link
Latest commit f46f66ea4e319975578d70e465f50ceaff0ba392
Skn0tt commented 1 year ago

Hi @KevinEdry, this is an amazing PR! Thank you so much - I love how you improved the docs. It's quite big, so reviewing will take a couple iterations, but we'll get there.

coveralls commented 1 year ago

Coverage Status

coverage: 82.464%. remained the same when pulling e2f0146bf39a0e66f5a4dc9f067004fed20010c7 on KevinEdry:main into 9344abf9f3c5235736d9802a43ade2fbb0230b6a on quirrel-dev:main.

Skn0tt commented 1 year ago

I hope it's okay that i'm committing onto your branch! I figured that's the fastest way to make progress. I'll comment about the changes i'm making and the rationale behind it, feel free to leave comments about anything.

In https://github.com/quirrel-dev/quirrel/pull/1141/commits/5b430d7a4d379950e9a757b33396b1e5c72df578, i've reworked the quirrel/next13 implementation to be based on the others we have in that directory. I've also changed the user-facing signature to be in-line with how quirre/sveltekit works: https://docs.quirrel.dev/api/sveltekit This means that devs won't destructure the return value of Queue, but instead they'll export it twice. I've also renamed the file to quirrel/next-app, since next13 will probably grow out-of-date pretty soon :D

Skn0tt commented 1 year ago

I've reverted the changes to the CronJob API docs. I don't think we need to differentiate between Next.js App Router / Pages Router on that page - it's not framework-specific, and Next.js isn't the only framework that's supported by Quirrel.

KevinEdry commented 1 year ago

It is totally fine for you to commit on my branch, I didn't notice you had a similar implementation in quirre/sveltekit, I would probably would go with that. For the CronJob, I agree that this isn't framework specific, but I do think the docs should also reflect the implementation for every framework, such as a tab selector for each framework and it's implementation. (I might do that later).

Skn0tt commented 1 year ago

in https://github.com/quirrel-dev/quirrel/pull/1141/commits/1d51c94ab121f8e24505a3aaaa1cd7882df0919f, i've removed the docker-compose development docs. Quirrel has a local dev mode, and that should be used locally. I think that docker-compose is way too complex for most usecases of this, so I don't want to highlight it in the docs. Appreciate the document, though!

KevinEdry commented 1 year ago

@Skn0tt I wonder if we also need to change the cron-detector since we are no longer importing from quirrel/next.

Skn0tt commented 1 year ago

but I do think the docs should also reflect the implementation for every framework

I want to keep the documentation small and concise, and i'm unsure if another documentation for CronJob for every single framework adds much value.

Skn0tt commented 1 year ago

I wonder if we also need to change the cron-detector

I don't think so, the cron-detector isn't hard-coded on framework names, but matches on quirrel/*: https://github.com/quirrel-dev/quirrel/blob/305e4243a9286f1aed3c6792063790d07d946a9d/src/cli/cron-detector.ts#L20

Skn0tt commented 1 year ago

This is now in a state where i'm happy with it. @KevinEdry could you give it another read and let me know if there's anything you think needs changing?

KevinEdry commented 1 year ago

@Skn0tt LGTM, the only thing I would change is to add the Payload to the CronJob, but it is your call, I will manage either way in my project and my specific use-case. I hope these changes will help people use this lib more, I found it really useful for my purposes.