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

When specifying CronItemOptions, backfillPeriod is not optional in TypeScript #495

Open
danieldiekmeier opened this issue Oct 6, 2024 · 1 comment
Labels
good first issue Good for newcomers

Comments

@danieldiekmeier
Copy link
Contributor

Summary

I'm defining my cronList as an array of CronItems, so that I can later pass them through parseCronItems. This has worked great! (Thank you very much for the library!)

Today, I wanted to add maxAttempts: 1 to one of my CronItems, and created an options object for that. TypeScript started complaining that the options must have a backfillPeriod key.

Steps to reproduce

Use this code:

import { type CronItem } from 'graphile-worker'

const cronList: CronItem[] = [
  {
    task: 'example',
    match: '* * * * *,
    identifier: 'example',
    options: {
      maxAttempts: 1,
      // TypeScript: Property 'backfillPeriod' is missing in type '{ maxAttempts: number; }' 
      //             but required in type 'CronItemOptions'. ts(2741)
    },
  },
}

Expected results

I'd expect the key to be optional, like the other options in CronItemOptions.

Actual results

At runtime, this seems to work correctly and I don't see any issues. The problem is only the type error.

Additional context

  • Node.js 20.17.0
  • TypeScript v5.6.2

Possible Solution

I think the problem is that Graphile Worker uses the same interface internally and externally. If I understand correctly, the backfillPeriod is defaulted to 0 here:

worker/src/crontab.ts

Lines 151 to 153 in 436e299

if (!backfillPeriod) {
backfillPeriod = 0;
}

So it makes sense that internally, backfillPeriod is always defined. But since this runtime default exists, I think there should be a slightly different interface to the outside.

@benjie
Copy link
Member

benjie commented Oct 7, 2024

Having a different interface for internal types makes sense (leaving the exposed interfaces with the same names). I think I already knew about this but it seemed so minor I didn't bother resolving it. Feel free to send through a PR 👍

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

No branches or pull requests

2 participants