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

Pass background tasks as Promise to new waitUntil option #103

Merged
merged 1 commit into from
May 8, 2024

Conversation

richardscarrott
Copy link
Contributor

@richardscarrott richardscarrott commented May 8, 2024

When running in a serverless environment such as Cloudflare Workers any background tasks such as cache revalidation and value migration aren't guaranteed to complete successfully because the serverless process is likely to be killed once it's handled the request.

This PR adds a new waitUntil option which allows call sites to pass in a callback accepting a promise representing background tasks.

In practice on CF Workers this would look like this:

export default {
  async fetch(request, env, ctx) {
    const result = await cachified({
      key: 'test',
      cache: lru,
      async getFreshValue() {
        const response = await fetch(
          `https://jsonplaceholder.typicode.com/users`,
        );
        return response.json();
      },
      ttl: 300_000,
      waitUntil: ctx.waitUntil.bind(ctx).
    });

    return new Response(result);
  },
};

https://developers.cloudflare.com/workers/runtime-apis/context/#waituntil

#71 (comment)

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This makes sense to me. Thanks for being so thorough!

@kentcdodds kentcdodds requested a review from Xiphe May 8, 2024 12:29
@AdiRishi
Copy link
Contributor

AdiRishi commented May 8, 2024

Amazing PR, you tacked an issue that's been at the back of my mind for almost half a year now!

If you don't mind a question, why did you bind ctx to waitUntil in your example? Is this how you expect we would need to pass in that function? Looking at the code I would think it's enough to simply pass in ctx.waitUntil

@richardscarrott
Copy link
Contributor Author

Hi @AdiRishi -- if you don't bind it blows up (or at least it did when I first tried workers ~a year ago) -- I presume it tries to reference itself on this.

Interestingly enough, if you're using CF Pages it's already bound -- I think here https://github.com/cloudflare/workers-sdk/blob/e57758f8ea39d296a6e11fac2c3629bf023ba850/packages/wrangler/templates/pages-template-worker.ts#L155 -- but if you're using Workers directly or CF Pages in advanced mode you need(ed) to bind.

Thanks for your work on the KV adapter -- just about to give it a go.

@AdiRishi
Copy link
Contributor

AdiRishi commented May 8, 2024

Hi @AdiRishi -- if you don't bind it blows up (or at least it did when I first tried workers ~a year ago) -- I presume it tries to reference itself on this.

Very interesting, I'm curious to give this a shot myself and see what the behavior is like.

Thanks for your work on the KV adapter -- just about to give it a go.

Glad its turned out to be useful! If anything goes wrong or you find improvements to be made don't hesitate to create an issue / let me know 😄

@kentcdodds kentcdodds merged commit 3edb5dd into epicweb-dev:main May 8, 2024
4 checks passed
Copy link

github-actions bot commented May 8, 2024

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants