Allow TS users to overload `addJob` (take two)

[keithlayne]

It would be nice for TS users who enqueue jobs from within tasks (or via the addJob function returned by run) to have payload types checked at the call site.

NOTE: This has nothing to do with validating payloads. This feature would be only useful insofar as the payload types are correct, and would only provide type checking at addJob call sites.

In my current setup, I use runtypes to derive both payload validators and the static types that I would use in these overloads. In this way there is a single source of truth, and when the runtime type is updated, TS will trickle that down to all call sites. This feels pretty robust.

Adding this feature would give additional type checks and an improved editor experience.

The requirement for this to work is that you pass a string literal (or a value typed as a non-union string literal) as the first parameter. It's assumed that if you add an entry for a task that the payload will be a required argument. You can however make a payload type nullable if you wish.

Passing any value that isn't inferred as a string key of the TaskPayload should result in falling back to the original function type. The reason for the non-union key requirement is that in that case (with current implementation) you could satisfy the type checker with an incorrect payload type.

This is (almost) fully implemented in the playground link below:

Playground Link

[benjie]

I... Don't want to maintain TypeScript types that look like that. Props on getting it working, but I'd like to see a much simpler solution.

Can we do something like add overloads via declaration merging? It seems a lot more straight-forward.

declare module 'graphile-worker' {
  interface FooPayload {
    bar: string;
    baz: string;
  }
  interface AddJobFunction {
    (task: "foo", payload: FooPayload) => Promise<void>;
  }
}

(Not tested.)

[keithlayne]

I thought I addressed some stuff from my experiments in #56 but looking back I was just rambling.

The main drawback to that approach is that the original signature would look something like this in interfaces.ts:

export interface AddJobFunction {
   (identifier: string, payload?: any, options?: TaskOptions): Promise<Job>;
}

(That just changes the current version from type to interface)

Special TS rules will put the overloads like in your example first in the lookup because of the single string literal arg, which is great. You would get autocompletion for your overloads, which is great. What's not great is that as soon as your payload at the callsite doesn't match your overload, it will be treated as any and you will get runtime errors while typechecking successfully ☹️

I agree the types are gnarly in my version, but it does seem to be complete and correct according to all my checks. It's possible I've missed some cases, I hope not. It just comes down to whether or not you are willing to make that tradeoff of complexity of type for (IMO) an improved end user experience.

I think just merging the payload interface is really easy to use. You maintain a bunch of libraries, so I'm definitely preaching to the choir here, but IMO libs should hide complexity for the sake of users, and I think this fits that pattern. I would be thrilled if this impl could be simplified. There could be some tradeoffs to simplify the types at the cost of total correctness.

Anyway, I understand if you think this isn't a good idea, but thanks for at least looking at it!

[benjie]

Thanks for your reasoned discussion. I'm not comfortable with the types you've laid out, but I'm also not happy with the declaration merging approach.

What do you feel is wrong with this approach? It's a simplified form of yours.

Note it's expected that the payload should always be an object, but technically it could be any valid JSON subelement (null, boolean, number, string, object, array). I'm perfectly happy for us to limit it to being just objects and arrays though, if that helps. Or even just objects if that helps more.

[benjie]

(Updated version of your playground, where the task payloads are expected to be objects.)

[keithlayne]

My examples for testing were not great. 😐

The main issue with the simplified version is that it breaks the current API, which I assumed (maybe wrongly since it's early-ish in this project?) was an instant no-go.

Supporting the current API is really half the complexity of my implementation -- currently the paylod is optional, which I think is reasonable for some tasks. Also, the simplified version doesn't allow arbitrary string task names any more.

As far as payload types go, I assumed valid JSON was okay. I think it's reasonable to restrict the payload types, but I don't see it impacting the types really, so I don't see it as necessary. (It's actually hard to accept only non-array objects in TS.)

The other source of complexity is the single-string-literal restriction. If you specify in the docs that addJob should only be called with string literals in TS, that could be sufficient, and I think that is what 99% of people are going to do anyway. There are some other cases, but I think that could be reasonably relaxed and handled with documentation. Basically if the type of the taskname is too wide you won't get real type safety, which is IMO undesirable, but pretty easy to fix at call sites as long as you're not going for JS-like dynamism.

[benjie]

I'm okay with breaking (strengthening) the types of the API in 0.3.0. We're still 0.x so churn like that is expected. As for expecting dynamic arguments to work with addJob there's always any or // @ts-ignore if you need that. Regarding optional payload, let's get rid of that from the TypeScript. (Technically the implementation will still have = {} but that's not relevant to the types.) I'd like to allow omitting it, but it's not clear how to do that without deep generics and helpers which I'd very much like to avoid.

[keithlayne]

This playground shows a compromise where the single-literal restriction is lifted. It's much simpler, but it retains compatibility with the current API. It also has an example at the bottom of the dangers of widenting the type fo the task name.

I don't know why I didn't think of this before, but instead of changing addJob you could instead add another function with the overloadable interface. I'm kicking myself because in TS if you want to add overloads you should probably always consider using a different function instead. This function could be a reference or just delegate to the original implementation, then you could use your simplified version for the interface.

In fact, it's entirely reasonable for TS users to implement this in their projects without changing worker at all. It would be a little more work, but I think I could achieve what I want by just wrapping addJob in my project. Soooooo...what do you think? :)

[keithlayne]

Okay, was typing while you wrote that, it changes what I'd propose.

[keithlayne]

Making the payload required is a game changer.

[benjie]

You're right about adding addJob wrappers; but we really want the payload type shared with the tasks themselves. Really I want:

interface Types {
  task1: PayloadTask1;
  task2: PayloadTask2;
  [otherTask: string]: unknown;
}

That'd be compatible with the current API, but at the moment I don't think TypeScript has a way of saying "any key except for the keys we already defined in this object" in a way that works with declaration merging.

[keithlayne]

This Is what it looks like when the payload is required. Or unknown like your example is great.

[keithlayne]

This shows a potential way to have jobs with no payload.

[keithlayne]

This does no-payload jobs at the cost of an extra helper, but with the advantage of no overloads, which improves the error messages.

[keithlayne]

I came up with some much simpler helpers for checking for unions. This playgroun shows how they can be used to fall back to the unknown case when the task name is typed as a union of string literals. Doesn't buy any real type safety, but can give you a hint by the type of a payload that you're doing it wrong.

We could also use undefined instead of never to make payload types optional. I don't know how I feel about that. It would mean another pseudo-overload in the AddJobArgs helper.

[keithlayne]

Last one today, I promise. This much more simply solves the union-of-literals constraint.

[keithlayne]

Wow, maybe I'm the only one who cares? I saw your CTA in discord a while back. #crickets

[jhoch]

I'm also interested in type safety on my payloads when calling addJob. I haven't gotten a chance to look through all of the playground links above, but I was thinking about adding a wrapper function with something like @benjie 's suggestion above.

Original suggestion

interface PayloadByJob {
  foo: { a: number };
  bar: { b: string };
}

declare function addJob<K extends keyof PayloadByJob>(
  identifier: K,
  payload: PayloadByJob[K],
  options?: TaskOptions
): Promise<void>;

Alternative:

interface FooJob {
  identifier: 'foo';
  payload: {
    a: number;
  };
}
// ...
type Job = FooJob | BarJob | ...

declare function myWrapper(
  job: Job,
  options?: TaskOptions,
): Promise<void>;

And then it would make sense to declare PayloadByJob/Job adjacent to where the tasks are registered with graphile-worker.

My two cents!

[jhoch]

One disadvantage with the wrapping-addJob approach: the wrapper method isn't available on helper.

[keithlayne]

Yeah. That's a big trade-off. It's way easier to go the wrapper route, but you don't get as much benefit IMO.

[benjie]

Looks like TypeScript may have advanced since I last looked into this. Here's my proposal based on what @keithlayne previously propsed, it doesn't involve complex type gymnastics and the only breaking change to the types is that payload is now a required argument to addJob.

Add a lookup type that maps from task identifier to the type of the payload. We'll export this, and users can extend it with declaration merging:

interface TaskPayloadByIdentifier {
  [taskIdentifier: string]: unknown;
}

addJob is simple, it can infer the relevant type to use from the first argument:

function addJob<T extends string = string>(
  identifier: T,
  payload: TaskPayloadByIdentifier[T],
  spec?: TaskSpec
) {
  /* ... */
}

The task type is slightly harder, but we'll just recommend that people pass the identifier as a generic, and if they forget then the payload will be an unknown:

export type Task<TaskIdentifier extends string = string> = (
  payload: TaskPayloadByIdentifier[TaskIdentifier],
  helpers: JobHelpers
) => void | Promise<void>;

Then in user code, you'd add types via declaration merging:

declare module 'graphile-worker' {
  interface TaskPayloadByIdentifier {
    user__updated: { id: number };

    send_email:
      | { message: string | null; subject: string | null; from?: string }
      | { body: string };
  }
}

And when you declare a task you'd pass the identifier as a generic:

const task: Task<'send_email'> = (payload, helpers) => {
  /* ... */
}

Playground link

If there's general consensus on this I'll get it implemented.

[benjie]

We could also type taskList automatically for API users (as opposed to CLI users). However; I seem to be having some issues with the types there:

Playground Link

Specifically line 85 and line 88 have issues, even though the types inside the functions are detected correctly.

[benjie]

This is the closest I can get, but it means that you can't add tasks to a taskList without first declaration merging them into the payload types:

Playground link

import { JobHelpers, TaskSpec } from "graphile-worker";

// Central register of payload types. This map to unknown would be 
// bundled with GraphileWorker and would mean this is a non-breaking
// change.
interface TaskPayloadByIdentifier {}
type TaskPayload<TaskIdentifier extends string = string> = TaskIdentifier extends keyof TaskPayloadByIdentifier ? TaskPayloadByIdentifier[TaskIdentifier] : unknown;


function addJob<T extends string = string>(
  identifier: T,
  payload: TaskPayload<T>,
  spec?: TaskSpec
) {
  /* ... */
}

export type Task<TaskIdentifier extends string = string> = (
  payload: TaskPayload<TaskIdentifier>,
  helpers: JobHelpers
) => void | Promise<void>;

type TaskList = {
  [T in keyof TaskPayloadByIdentifier]?: Task<T>;
}

export const taskList: TaskList = {
  user__updated(payload, helpers) {
    // Here we know the type so all is well
    console.log(payload.id); // number
  },
  send_email(payload, helpers) {
    // Supports unions, e.g. for versioned payloads
    if ("message" in payload) {
      console.log(payload.message, payload.subject, payload.from); // string|null, string|null, string|undefined
    } else {
      console.log(payload.body); // string
    }
  },
  /*
  // Can't add untyped fields because `{[T: string]: Task}` overwrites the types used above. So we can't have this:

  foo(payload, helpers) {
    console.dir(payload);
    console.log(payload.anything_at_all); // Error: unknown
  }
  */
}

[keithlayne]

Contravariance strikes again...

type test = Task<'user__updated'> extends Task<string> ? true : false; // false

[keithlayne]

Sorry if that wasn't clear. If Task was covariant WRT the type param, you could do (roughly):

type TaskList = {
  [T in keyof TaskPayloadByIdentifier]?: Task<T>;
} & {
  [identifier: string]: Task | undefined;
}

But all the things must be compatible with the index signature.

Dynamic keys and index signatures are just not very TS-friendly. It's super convenient to define things by name in the task list like that and get the contextual typing of the payload. But TS much prefers arrays of things with static keys. I'm not sure if applying that can get your desired UX in this case, but it would be a breaking change.

FWIW, I'm not a huge fan of contextual typing here. It reminds me kinda of React.FC, which is problematic in sorta different ways.

This is basically my worker entrypoint:

import { run } from 'graphile-worker';
import * as taskList from './tasks';
run({ connectionString, taskList}).catch((err) => { /* stuff */ }

I think the tasks should just be functions that annotate their payload types. In fact, my task modules all 1) Define the payload type 2) export the function using the payload type. In particular I don't think the task name should be part of the static type of a task.

[moltar]

I think just relaxing the payload type from unknown to any could allow for HOF use that is typed.

Right now, even when I create a class to handle the jobs:

class TaskA<P extends {}> {
  public readonly name = this.constructor.name

  async run(payload: P, helpers: JobHelpers) {
    helpers.logger.info(JSON.stringify(payload))
  }

  add(payload: P) {
    // Or add a job to be executed:
    return quickAddJob(
      // makeWorkerUtils options
      { connectionString: config.DATABASE_URL },

      // Task identifier
      this.name,

      // Payload
      payload,

      {
        queueName: 'new',
      },
    )
  }
}

const task = new TaskA<Payload>()

When I assign it to taskList, TypeScript complains:

const runner = await run({
    connectionString: config.DATABASE_URL,
    concurrency: 1,
    // Install signal handlers for graceful shutdown on SIGINT, SIGTERM, etc
    noHandleSignals: false,
    pollInterval: 10000,
    // you can set the taskList or taskDirectory but not both
    taskList: {
      [task.name]: task.run,
    },
  })

Type '{ [x: string]: (payload: Payload, helpers: JobHelpers) => Promise<void>; }' is not assignable to type 'TaskList'.
  Index signatures are incompatible.
    Type '(payload: Payload, helpers: JobHelpers) => Promise<void>' is not assignable to type 'Task'.
      Types of parameters 'payload' and 'payload' are incompatible.
        Type 'unknown' is not assignable to type 'Payload'.ts(2322)

[benjie]

I think the tasks should just be functions that annotate their payload types.

@keithlayne If we do this, surely we're losing the guarantee that a certain named task uses a particular payload?

I think just relaxing the payload type from unknown to any could allow for HOF use that is typed.

This is one of the leading options for me. I'm actually thinking we should drop the fallback signature and just require declaration merging for everything - this seems to be what Express does for Request.User for example:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/a095649c044569b36b6547059788c838e1047d2f/types/express-serve-static-core/index.d.ts#L18-L26

[keithlayne]

Maybe so. Gotta think about it.

I guess I always assumed that there would be declaration merging where you'd explicity attach a name to a payload, and that way you can typo the name. I always use the exported function name as the task name.

I guess you could make a linter rule for that, but I've never done that, and it seems like more work. But declaration merging is writing declarations by hand - mistakes there won't be checked generally. Would love it if function calls could effectively merge the types, then you could just addTask or whatever (with name) and it would all just work out.

[elnygren]

I'm also interested in this, my current solution that I came with completely independently of your discussions looks like this:

import type { JobHelpers, Task } from 'graphile-worker'

export type MyJob = 
  | { t: 'foo'; payload: { a: string } } 
  | { t: 'bar'; payload: { b: string } }

/** Typesafe handler for our tasks with correctly typed payloads. */
const taskHandler = async (task: MyJob, helpers: JobHelpers) => {
  switch (task.t) {
    case 'foo':
      // idea would be to call a task function here 
      console.log(task.payload.a)
      return
    case 'bar':
      console.log(task.payload.b)
      return
  }
}

/**
 * Tasklist for graphile-worker
 * We wrap our type safe tasks to be compatible with graphile-worker's typing of
 * `Task = (payload: unknown, helpers: JobHelpers) => void | Promise<void>`
 *
 * XXX: Take extra care that `t` matches with the taskList key.
 */
export const taskList: { [x in MyJob['t']]: Task } = {

  // yes, casting to `any` is nasty, but it only happens in this one wrapper and everything else is typesafe
  foo: async (payload, helpers) => taskHandler({ t: 'foo', payload: payload as any }, helpers),
  bar: async (payload, helpers) => taskHandler({ t: 'bar', payload: payload as any }, helpers),
}

// and finally, adding tasks to queue with type safety:
export const addJobToQueue = async (task: MyJob, spec?: TaskSpec) => {
  workerUtils.addJob(task.t, task.payload, spec)
}

I think it would be a very powerful feature to be able to define all the tasks at the type level and then implement them with type safety. Especially when the same project controls adding & handling tasks.

Also taking this further, I guess it would be totally possible to use zod or similar to actually validate that payload is of the correct type in my helpers, so implementing the tasks becomes even nicer.

[JonParton]

Hey @elnygren - just flagging I also did similar but added the ability to use task folders also and used code gen to generate the type safety with Zod checking at run time! Might be some inspiration

https://github.com/graphile/worker/issues/264#issuecomment-1294912088