-
Notifications
You must be signed in to change notification settings - Fork 67
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
added nextjs 13 app router native support & docs improvements. #1141
Conversation
✅ Deploy Preview for quirrel-docs canceled.
|
👷 Deploy request for quirrel-development-ui pending review.Visit the deploys page to approve it
|
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. |
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 5b430d7, i've reworked the |
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. |
It is totally fine for you to commit on my branch, I didn't notice you had a similar implementation in |
* | ||
* export const POST = sampleCron; | ||
*/ | ||
export function CronJob( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add here a <Payload>
generic and pass it to the handler or the Queue.
export function CronJob<Payload>(
route: string,
schedule: string,
handler: QuirrelJobHandler<Payload>,
options?: QuirrelOptions<Payload>
) {
return Queue(route, handler, options);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CronJobs don't receive any kind of payload, so there's no need for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use a cronjob with a payload, the cron is starting the job, the job is making an api request, and if the response contains a cursor, it invokes the same job with the cursor:
export const { POST, queue: companiesQueue } = CronJob<Payload>(
"/api/queue/companies",
"0 0 * * 1",
async (payload?: Payload) => {
try {
const fetchUrl =
payload?.cursor != null
? payload.cursor
: "https://api.random-example.com/something";
const res = await fetchApi<Ticker>(fetchUrl);
const { results, next_url } = tickersSchema.parse(res);
if (next_url != null) {
companiesQueue.enqueue({ cursor: next_url });
}
return;
} catch (error) {
console.log({ error });
throw error;
}
}
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! I'd have never imagined anyone to use a CronJob like this 😁 I think if we documented this, it'd be very non-obvious how it works in most cases. So i'd prefer to not document it. I'm sure you'll find a workaround for your specific case :D
in 1d51c94, 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! |
@Skn0tt I wonder if we also need to change the cron-detector since we are no longer importing from |
I want to keep the documentation small and concise, and i'm unsure if another documentation for |
I don't think so, the cron-detector isn't hard-coded on framework names, but matches on quirrel/src/cli/cron-detector.ts Line 20 in 305e424
|
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? |
@Skn0tt LGTM, the only thing I would change is to add the |
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
andCronJob
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 bothPOST
and the queue instance in one go, and alias the queue instance with a meaningful name.Queue Usage:
CronJob Usage:
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
next13.ts
file to support nextjs 13 app directory natively.docs
BEGIN_COMMIT_OVERRIDE
feat: added nextjs 13 app router native support & docs improvements
END_COMMIT_OVERRIDE