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

Can not use 1.2.0 without bullmq depenedency #202

Closed
malisetti opened this issue Dec 18, 2020 · 24 comments
Closed

Can not use 1.2.0 without bullmq depenedency #202

malisetti opened this issue Dec 18, 2020 · 24 comments
Milestone

Comments

@malisetti
Copy link

malisetti commented Dec 18, 2020

My project just uses 'bull' not ''bullmq'. When i try to compile the project with tsc i get the following errors.


node_modules/bull-board/dist/@types/app.d.ts:2:43 - error TS2307: Cannot find module 'bullmq' or its corresponding type declarations.

2 import { Job as JobMq, JobsOptions } from 'bullmq';
                                            ~~~~~~~~

node_modules/bull-board/dist/queueAdapters/bullMQ.d.ts:1:28 - error TS2307: Cannot find module 'bullmq' or its corresponding type declarations.

1 import { Job, Queue } from 'bullmq';
                             ~~~~~~~~


Found 3 errors.

@felixmosh
Copy link
Owner

Thank you for reporting the issue, We need to think how to solve this issue.

From one side we don't want to install both of the libs from the other side we do want to use it's types.

@johnfazzietempus
Copy link

Also getting this error.

@felixmosh
Copy link
Owner

PR's are always welcome :]

@jsbrain
Copy link

jsbrain commented Feb 16, 2021

I think probably splitting the types into two different dist folders should do the trick, with one being the default.

So if you want to use it will bull, doing:

import { BullAdapter } from 'bull-board'

is sufficient, where as if you want to import the BullMQ packages, you'd have to do:

import { BullMQAdapter } from 'bull-board/dist/bullmq/queueAdapter/...' // <- whatever file

and same works for Bull as well:

import { BullAdapter } from 'bull-board/dist/bull/queueAdapter/...' // <- whatever file

so the "default" only requires/exports the types from either one of the libraries but you can get both, when you need em ...

@felixmosh
Copy link
Owner

@jsbrain thank you for the suggestion,
Unfortunately, it is not so simple, since there are several places which uses both of the types, such as https://github.com/vcapretz/bull-board/blob/master/src/%40types/app.ts.

@jsbrain
Copy link

jsbrain commented Feb 16, 2021

@felixmosh yes sure, you'd either have to exclude two sets of types for each library, or better, transform those types to conditional/generic types so the actual final types are defined by one single type param which is either bull or bullmq.

I'm currently super busy at work so idk if I'm able to contribute much but I will try to provide at least an example type soon which could be used as template for the other types :)

@jsbrain
Copy link

jsbrain commented Feb 16, 2021

Alright, so I spend some time digging around and partially solved the "issue" I guess. So the main problem here, as far as I can tell, is mainly the differentiation between the Job and JobMq type.

I think the best solution to fix this is, to rewrite everything to using generic types only. So the actual Job type will be passed as generic type to each function, class, other types so those actually library specific types (like Job, JobOptions, etc.) will only be imported in bull.ts and bullMQ.ts respectively.

Example:

Instead of doing it like this:

// static types
export interface QueueAdapter {
  readonly client: Promise<Redis.Redis>
  readonly readOnlyMode: boolean

  getName(): string

  getJob(id: string): Promise<Job | JobMq | undefined | null>

  getJobs(
    jobStatuses: JobStatus[],
    start?: number,
    end?: number,
  ): Promise<(Job | JobMq)[]>

  getJobCounts(...jobStatuses: JobStatus[]): Promise<JobCounts>

  clean(queueStatus: JobCleanStatus, graceTimeMs: number): Promise<any>
}

we do it like this:

// generic types
export interface QueueAdapter<LibrarySpecificJob> {
  readonly client: Promise<Redis.Redis>
  readonly readOnlyMode: boolean

  getName(): string

  getJob(id: string): Promise<LibrarySpecificJob | undefined | null>

  getJobs(
    jobStatuses: JobStatus[],
    start?: number,
    end?: number,
  ): Promise<LibrarySpecificJob[]>

  getJobCounts(...jobStatuses: JobStatus[]): Promise<JobCounts>

  clean(queueStatus: JobCleanStatus, graceTimeMs: number): Promise<any>
}

instead of defining functions like this:

const formatJob = (job: Job | JobMq): app.AppJob => {
  const jobProps = job.toJSON()

  return {
    id: jobProps.id,
    timestamp: jobProps.timestamp,
    processedOn: jobProps.processedOn,
    finishedOn: jobProps.finishedOn,
    progress: jobProps.progress,
    attempts: jobProps.attemptsMade,
    delay: job.opts.delay,
    failedReason: jobProps.failedReason,
    stacktrace: jobProps.stacktrace,
    opts: jobProps.opts,
    data: jobProps.data,
    name: jobProps.name,
    returnValue: jobProps.returnvalue,
  }
}

we do it like this:

// Used to be able to inherit actual JobOptions type from Job by using index
// e.g. Job['opts'] => JobOptions | JobsOptions, depending on Job type from 'bull' or 'bullmq'
type JobMock = {
  opts: Record<string, any>
  toJSON: () => any
  retry: () => any
}


const formatJob = <Job extends JobMock, AddProps>(
  job: Job,
): app.AppJob<Job['opts'], AddProps> => {
  const jobProps: ReturnType<Job['toJSON']> = job.toJSON()

  // TODO: Should be improved to return actual present values only ...
  return ({
    id: jobProps.id,
    timestamp: jobProps.timestamp,
    processedOn: jobProps.processedOn,
    finishedOn: jobProps.finishedOn,
    progress: jobProps.progress,
    attempts: jobProps.attemptsMade,
    delay: job.opts.delay,
    failedReason: jobProps.failedReason,
    stacktrace: jobProps.stacktrace,
    opts: jobProps.opts,
    data: jobProps.data,
    name: jobProps.name,
    returnValue: jobProps.returnvalue,
  } as unknown) as app.AppJob<Job['opts'], AddProps>
}

This means, those imports:

import { Job, JobOptions } from 'bull'
import { Job as JobMq, JobsOptions } from 'bullmq'

have to be removed from all files but bull.ts and bullMQ.ts. In those two files, we would basically create a copy of the current index.ts, wrapping it in a factory function that will pass the library specific types to the remaining (generic) code. So dependent on which library the user wants to use, he would either use bull.ts or bullMQ.ts as "entry point" and the code would adjust accordingly.

The main index.ts could then just import export the default entry point (for example bull.ts) and if the user want to opt-in (or opt-out) to 'bullmq', he must chose this entry point specifically.

Using this technique, the compiler won't complain as long as you'd use only one of the both entry points (obviously).

I actually started to rewrite some of the functions in a very naive way that actually should work but can probably be improved a lot. So for example for the app.ts file you @felixmosh mentioned in your last message, it could look like:

// import { Job, JobOptions } from 'bull' // <- we don't need you anymore 😭
// import { Job as JobMq, JobsOptions } from 'bullmq' // <- don't need you either 😢
import React from 'react'
import * as Redis from 'ioredis'
import { Status } from '../ui/components/constants'

export type JobCleanStatus =
  | 'completed'
  | 'wait'
  | 'active'
  | 'delayed'
  | 'failed'

export type JobStatus = Status

export type JobCounts = Record<JobStatus, number>

export interface QueueAdapter<BullJob> {
  readonly client: Promise<Redis.Redis>
  readonly readOnlyMode: boolean

  getName(): string

  getJob(id: string): Promise<BullJob | undefined | null>

  getJobs(
    jobStatuses: JobStatus[],
    start?: number,
    end?: number,
  ): Promise<BullJob[]>

  getJobCounts(...jobStatuses: JobStatus[]): Promise<JobCounts>

  clean(queueStatus: JobCleanStatus, graceTimeMs: number): Promise<any>
}

export interface QueueAdapterOptions {
  readOnlyMode: boolean
}

export interface BullBoardQueue<BullJob> {
  queue: QueueAdapter<BullJob>
}

export interface BullBoardQueues<BullJob> {
  [key: string]: BullBoardQueue<BullJob>
}

export interface ValidMetrics {
  total_system_memory: string
  redis_version: string
  used_memory: string
  mem_fragmentation_ratio: string
  connected_clients: string
  blocked_clients: string
}

export type GenAppJob<JobOpts> = {
  id: string | number | undefined
  timestamp: number | null
  processedOn?: number | null
  finishedOn?: number | null
  stacktrace: string[] | null
  opts: JobOpts
  delay: number | undefined
  returnValue: string | Record<string | number, any> | null
}

// NOTE: The additional props type probably could be omitted, depending on the actual usage of the props
export type AppJob<JobOpts, AddProps> = AddProps extends Record<string, never>
  ? GenAppJob<JobOpts>
  : GenAppJob<JobOpts> & AddProps

export interface AppQueue<JobOpts, AddProps> {
  name: string
  counts: Record<Status, number>
  jobs: AppJob<JobOpts, AddProps>[]
  readOnlyMode: boolean
}

export type SelectedStatuses<JobOpts, AddProps> = Record<
  AppQueue<JobOpts, AddProps>['name'],
  Status
>

export interface QueueActions<JobOpts, AddProps> {
  promoteJob: (
    queueName: string,
  ) => (job: AppJob<JobOpts, AddProps>) => () => Promise<void>
  retryJob: (
    queueName: string,
  ) => (job: AppJob<JobOpts, AddProps>) => () => Promise<void>
  cleanJob: (
    queueName: string,
  ) => (job: AppJob<JobOpts, AddProps>) => () => Promise<void>
  retryAll: (queueName: string) => () => Promise<void>
  cleanAllDelayed: (queueName: string) => () => Promise<void>
  cleanAllFailed: (queueName: string) => () => Promise<void>
  cleanAllCompleted: (queueName: string) => () => Promise<void>
  setSelectedStatuses: React.Dispatch<
    React.SetStateAction<SelectedStatuses<JobOpts, AddProps>>
  >
}

Note the usage of conditional types as well, which could be beneficial in some places but maybe won't be necessary at all.

All changes I made and tried so far can be found here: https://github.com/jsbrain/bull-board/tree/example

Unfortunately, I have some very important deadlines to meet this month, so I probably won't be able to continue working on this (at least not this month). But I think for someone who is already familiar with the repo, the necessary changes should be pretty straightforward to implement.

I hope this helps, let me know if you see any issues with this approach that I may have missed 🤓

@felixmosh
Copy link
Owner

@jsbrain thank you 🙏🏼
I will review your approach at the up coming weekend.

@felixmosh
Copy link
Owner

@jsbrain I've checked your suggestions, there is one problem with it, we are not restrict the type of Job generic type to one of supported types.

This means that you can pass any type as Job and it will accept it & explode on usages (if in your passed type there was no "real" method / property).

@jsbrain
Copy link

jsbrain commented Mar 1, 2021

Hey @felixmosh, I don't really understand what you mean ... 😬
You mean my example doesn't allow both job types from bull and bullmq or what?

@felixmosh
Copy link
Owner

The problem is that it allows any form of Job object (since it is not restricted). In order to restrict the generics, we should use <Job extends BullJob | BullMQJob> (but we can't since we don't want to use them :))

@jsbrain
Copy link

jsbrain commented Mar 1, 2021

No, Job will always refer to the Job object of the respecting library. As we would need to pass it to the generic shared functions as a type argument, the type of Job would always match the respecting library, hence being fully restricted.

Because of that, we would have to use two different entry point files, one for bull and one for bullmq that uses the generic types to resemble the final types based on the respecting Job type from each library.

So:

import { anything } from 'bull-board/dist/bull.ts'
// -> Job references Bull.Job
import { anything } from 'bull-board/dist/bullmq.ts'
// -> Job references BullMQ.Job

So no problem there ... or did you mean something else?

@felixmosh
Copy link
Owner

A simple example,

If we write types like

QueueAdapter<Job> {
  getJob(): Job
}

This type accepts "all" object that will be passed... this means that someone will can

// usage of the adapter
import {BullMQAdapter} from 'bull-board/bullMQAdapter';

new BullMQAdapter<{notJobProp: string}>(); // this is the problem

@jsbrain
Copy link

jsbrain commented Mar 1, 2021

Well yes but no, you didn't fully get the approach. This will be solved by the entrypoint files. Let me try to explain again:

Shared, generic types, functions, classes will live in /lib/shared/* for example. So the QueueAdapter could be defined as:

// -> ./lib/shared/queueAdapter.ts
export type QueueAdapterGeneric<Job> {
  getJob(): Job
}

now let's define entrypoint A (bull):

// -> ./lib/bull.ts
import { Job } from 'bull'
import { QueueAdapterGeneric } from './shared/queueAdapter'

export type QueueAdapter = QueueAdapterGeneric<Job>

and entrypoint B (bullmq):

// -> ./lib/bullmq.ts
import { Job } from 'bullmq'
import { QueueAdapterGeneric } from './shared/queueAdapter'

export type QueueAdapter = QueueAdapterGeneric<Job>

now, the important step is to make sure that at final usage all required types are imported from the respecting entrypoint file, depending on if you decide to use bull or bullmq:

Final usage:

now let's define entrypoint A (bull):

// -> ./myproject/index.ts
import { QueueAdapter } from 'bull-board/dist/bull.ts'

export type QueueAdapter = QueueAdapterGeneric<Job>

new BullMQAdapter<{notJobProp: string}>(); // -> Error: Job has no property notJobProb of type string blabla

and everything is fine! 😀

@jsbrain
Copy link

jsbrain commented Mar 1, 2021

That way, you could also get rid of the "double naming" interfaces like BullMQAdapter and BullAdapter and that stuff ... you won't need it anymore as the used Adapter will always reference the right adapter of the lib you installed automatically.

@felixmosh
Copy link
Owner

felixmosh commented Mar 1, 2021

Ha, you are correct! When it comes to lib consumers it will work perfectly.

I've spoke about all the places (internally, like this) that uses QueueAdapterGeneric... they will need to specify the Job types.
We have 2 options

  1. use the Job | JobMQ - won't solve our issue
  2. pass a custom superset of Job - removes the whole point of generic usage, and won't be updated when bull / bullmq types will be changed.

@jsbrain
Copy link

jsbrain commented Mar 1, 2021

Well in cases where the Job object will be used specifically and will require a different usage based on whether it is Job or JobMQ, that will partially be correct. But as far as I can tell, there are only a few cases where this holds true. For those cases, there will have to be 2 functions, classes, for each library, which will be referenced again in the respecting entry point.

Other than that, you'll never actually need the respecting Job object and make a decision to know if it is Job or JobMQ type. All those decisions need to be made on a higher level, not in the actual type, function, class itself.

In cases where this gets a little tricky, like here: https://github.com/jsbrain/bull-board/blob/1533c8ea179e2b9740cd1dccc58d8b9af0c3f490/src/routes/queues.ts#L74
we can actually trick the TS compiler to make this work anyways by using something like this: https://github.com/jsbrain/bull-board/blob/example/src/%40types/global.ts.

So types will actually be updated with the core libraries and there is no need for custom supersets. It all comes down to properly structure the generics, create the right factory functions (if needed) and resemble the final type dependency graph in the entry points.

I'm not saying that wouldn't require some work, but the actual implementation should be very straightforward, will solve the problem, maintainability should be alright and library updates are no problem at all.

@felixmosh
Copy link
Owner

Will you able to make a compiling PR with your idea?
Thank you very much for the time you spend here 🙏🏼

@felixmosh
Copy link
Owner

BTW, how would you annotate QueueAdapterGeneric usages here?

@plakk2020
Copy link

Following this issue. Is there a resolution?

@felixmosh
Copy link
Owner

@plakk2020 you can install bullmq as your deps,
I Still didn't found any good solution...

@jsbrain
Copy link

jsbrain commented Mar 24, 2021

Hey @felixmosh, sorry I was super busy and then got quite sick 2 weeks ago. Now I have to catch up with the work of 2 weeks so I don't think I will be able to create a PR any time soon :/

@jsbrain
Copy link

jsbrain commented Mar 24, 2021

BTW, how would you annotate QueueAdapterGeneric usages here?

Like so

@felixmosh
Copy link
Owner

felixmosh commented Apr 27, 2021

Released in v2.0.0

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

No branches or pull requests

5 participants