Stricter typing on data passed to worker

[SimonSimCity]

Fixes #692.

I've set quite some types and think this is as complete as I can get it. I also tried to split up the work into meaningful commits, so going through them one-by-one should give the reviewer a good understanding of the individual type-refactorings I did.

I haven't yet put down the work for showing how the types work, other than the example. I can create some documentation around this but first want to know whether this goes into the right direction.

I found the function getQueues() not to pass on the name property. Maybe this is something that should be fixed.

[coveralls]

coverage: 100.0%. remained the same
when pulling d65dc2d on SimonSimCity:types
into 720e76d on timgit:master.

[timgit]

Thanks for the submission. You are certainly further along in experience with TS than me, so I'm still trying to understand the rationale behind some of this. At first glance, it appears that you'd like to restrict the allowed queues you interact with from a type perspective. This means that if you'd like to interact with a queue, you first would have had to configure your intent to do that. This doesn't mean you can't interact with a queue dynamically at run-time, but TS would at least warn you that you should have registered that queue first, and if typechecks are part of your CI, you would want this lack of registration to fail your build.

Another question I have would be the use of const schema = ctx.schema as 'name' in some of the tests, which also threw me off. I understand now what that does, but it was a misleading line at first.

Overall, I'm fine with this, provided it's opt-in and doesn't require all users to use it. If it were required, my reaction was that it adds too much complexity to get started.

[SimonSimCity]

Thanks for the review. I'll try to provide some insight into what I thought:

All existing code-bases should work without changes.

This is possible just to a certain extend. In codebases that didn't use any types, everything works fine, because the class PgBoss takes the type DefaultJobsConfig as default for its first generics-argument. This is basically as the types were before:

name: string
Input type: object | null | undefined
Output type: Json | undefined | void

The only exception to this rule, that I couldn't keep intact without substantial changes, is the types of earlier generic functions, like fetch, work, getJobById, findJobs and getSpy. Most of these generic types were just to set a type on the returned value, which now is tightly coupled to the name of the job and restricted (if types are used).

If types are used, the library will get a lot stricter than it has been earlier, but I think this is a good sign and can avoid common pitfalls, where a user accidentally assigns the wrong type to a worker - a type that is incompatible with what actually is passed to send.

Types should be ensured without avoidable type-casting.

This required some of the generic types to pass through to almost every part of the system, but it let's you have the assurance, that those types always are compatible. Might be quite an overhead if nobody actively maintaining is thoroughly proficient in TypeScript to the extend needed 😅

If a user decides to use types, they should be as strict as reasonably possible.

The function passed to work should implicitly have the same type as data is allowed to be passed into send, findJobs or schedule and fetch should return the same type as the work function can respond with, plus additionally possible internal error types (see InternalJobOutput). This requires the PgBoss instance itself to be able to know what types each job can take in and can return.

Having this allows users to have the work function as part of a separate repository doing the actual work, while having another repository, where the work is sent in (or scheduled) while sharing the types, being warned every type something is incompatible with the shared types.

As additional benefit, the library now "eats its own dogfood" in the way that the timekeeper is subject to the same type rules, and is always allowed without the user having to explicitly define it.

---

Before, the types could vary for the same name and the user was responsible to set up a typing network around these functions which help to limit the types to what the work function actually can provide - now, the library takes care of this 😎

For the tests, I didn't want to rewrite all of the tests, so I figured out that doing some quirx here and there is acceptable to have types but possibly different for each test.

Hope this gives you more insight on my thought process. Would be nice to hear what you think of these statements.

[timgit]

If you guys that were involved in the previous Typescript PRs, I would love to have a few more eyes and opinions on the DX being proposed.

@davbrito @bcomnes @kibertoad

[bcomnes]

I can take a look this week.

[simonbrunel]

Thanks @SimonSimCity, I'm really looking forward for stricter typing.

I'm wondering if the following usage will be possible:

// ~modules/core/core.types.ts
// Type intended to be augmented by other modules.
export interface JobSchema {}

// ~modules/core/core.module.ts
const queue = new PgBoss<JobSchema>({ ...  });

// ~modules/foo/foo.types.ts

declare module "~modules/core/core.types.js" {
  interface JobSchema {
    "queue-foo": { input: { a: string, b: number } };
  }
}

// ~modules/bar/bar.types.ts

declare module "~modules/core/core.types.js" {
  interface JobSchema {
    "queue-bar": { input: { c: boolean } };
  }
}

So we don't have to declare everything in a single location.

Also, could we make output optional when declaring job types ?

[SimonSimCity]

Also, could we make output optional when declaring job types?

Thanks for the improvement, that wasn't too hard to add 😎

The other question, of making the job definition as an interface, is something I don't know whether this is possible. Everything I tried so far failed (I even found a quite old discussion on that issue microsoft/TypeScript#15300). In my opinion, it makes for a better control if the place where you instantiate pg-boss knows of all possible types of jobs.

[simonbrunel]

Thanks @SimonSimCity

In my opinion, it makes for a better control if the place where you instantiate pg-boss knows of all possible types of jobs.

It depends on the project architecture. In my case, I decided to expose a generic job service as part of a "core" module, which does not know anything about other modules (queues are defined in each module). I may need to rethink this abstraction if augmenting an interface does not work.

[SimonSimCity]

Fair. I just found a way in which this would be doable:

import { JobStates, PgBoss } from '../../src'

export interface JobSchema {
  [key: string]: {
    input: object | null | undefined;
    output?: {
      [S in JobStates[keyof JobStates]]?: unknown;
    }
  },
  'queue-bar': { input: { c: boolean }, output: { completed: boolean } }
}

const queue = new PgBoss<JobSchema>({})

queue.work('queue-bar', (j) => Promise.resolve(j[0]!.data.c))
queue.work('abba', (j) => Promise.resolve(j[0]!.data))

But by this, you would make whatever string assignable to the first parameter of queue.work. I would want he last line should fail, because the job-type abba is never defined. It still works, because it fits the index-signature - something I had to add in order to have it fit the type definition.

[simonbrunel]

Indeed, it's not ideal because of the reason you mentioned. It also does not work well with code completion (i.e. queue.send(" does not show available queues, at least in VSCode).

This seems to workaround the index-signature error:

// pg-boss
export type JobsConfig<K extends PropertyKey> = Record<
  K,
  {
    input: object | null | undefined;
    output?: Record<JobStates, Json | undefined>;
  }
>;

export class PgBoss<
  Queues extends object = DefaultJobsConfig,
  Config extends JobsConfig<keyof Queues> = Queues extends JobsConfig<keyof Queues>  ? Queues : never,
  // ...
> {
  async send<K extends keyof Config>(
    name: K,
    data: Config[K]["input"],
  ): Promise<void> {
    //...
  }
}

I'm not sure if it's an acceptable solution but then this works (including module augmentation):

// main.ts
export interface JobsSchema {
  "queue-a": { input: { a: string; b: number } };
}

export const boss = new PgBoss<JobsSchema>();

// Code completion work as expected
boss.send("queue-a", { a: "a", b: 42 }); // no error 
boss.send("queue-c", { foo: "bar" }); // type error

[SimonSimCity]

Thanks for the help and code, and I think I've settled on something that is sufficient without further changes on the codebase by destructuring the interface before passing it as generics to the class. See the following code example, which I also added to the library (9da63f8#diff-ac8ef012db944f3e55c1fbb64c909520514ed7dff642295b6242a2d035d8ef07):

import { PgBoss } from '../../src'

interface JobsSchema {
  'queue-a': { input: { a: string; b: number } };
  // foo: { bar: string }; // <-- This line will fail because its not a valid job definition.
}

interface JobsSchema {
  'queue-b': { input: { a: string; b: number } };
}

export const boss = new PgBoss<{
  [S in keyof JobsSchema]: JobsSchema[S];
}>({})

// Code completion work as expected
boss.send('queue-a', { a: 'a', b: 42 })
// boss.send('queue-c', { foo: 'bar' }) // <-- This line will fail because the job-type is not defined.

This way, an error will occur at the instantiation of the PgBoss instance in case the programmer defined an invalid property on the interface, and you get proper typing on the functions on the instance. This way the error is at least as close to the origin of the source while still maintaining full type-safety 🎉

[simonbrunel]

Awesome! Thanks for looking at that use case.

[simonbrunel]

Out of curiosity, do you know why declaring JobStates and Events like this instead of:

export type JobStates =
  | "active"
  | "cancelled"
  | "completed"
  | "created"
  | "failed"
  | "retry"

export type Events = 
  | "bam"
  | "error" 
  | "stopped" 
  | "warning" 
  | "wip";

I find it makes usage a bit cumbersome, for example:

{ [S in JobStates[keyof JobStates]]: Json | undefined }

// instead of:

{ [S in JobStates]: Json | undefined }

// or just:

Record<JobStates, Json | undefined>

[SimonSimCity]

It allows for easier object definition (https://github.com/timgit/pg-boss/blob/master/src/index.ts#L16). Whether this was the original reason, I don’t know. I wouldn’t change those types without good reason because it would break backwards compatibility, as they’re party of the public interface of the library (they’re exported in the index.ts and are thereby not just an internal type).

[simonbrunel]

I didn’t mean to change it, just curious about this approach (I didn’t see the export of events). Now that your PR relies on these types, a small improvement to get rid of JobStates[keyof JobStates] could be:

export type JobState =
  | "active"
  | "cancelled"
  | "completed"
  | "created"
  | "failed"
  | "retry";

export type JobStates = Record<JobState, JobState>;

// ... and maybe the same for Event / Events

Anyway, feel free to ignore this, and apologies for jumping into this PR while not maintaining the library.

[SimonSimCity]

No need to apologize. Improvements and suggestions, also constructive criticism are always welcome 😎🎉

[bcomnes]

Sorry I mean to take a deeper pass at this and never did.