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

Stuck should not be separate from JobStatus #2723

Closed
jdforsythe opened this issue Apr 17, 2024 · 6 comments
Closed

Stuck should not be separate from JobStatus #2723

jdforsythe opened this issue Apr 17, 2024 · 6 comments

Comments

@jdforsythe
Copy link

This JobStatus | 'stuck' requires us to use that in our codebase.

https://github.com/OptimalBits/bull/blame/60fa88f08637f0325639988a3f054880a04ce402/index.d.ts#L261

const stateProms: Promise<JobStatus | 'stuck'>[] = jobs.map((j) => j.getState());

However, 'stuck' is one of the valid job statuses, according to the docs and code, so why isn't it just a part of the JobStatus union?

https://github.com/OptimalBits/bull/blob/develop/index.d.ts#L358-L371

Then our code can be simplified:

const stateProms: Promise<JobStatus>[] = jobs.map((j) => j.getState());
@manast
Copy link
Member

manast commented Apr 17, 2024

There is no such thing as "stuck" status in Bull. When a job stalls, it goes back to wait, it cannot be in any other state other than the ones provided by the typescript definition.

@manast manast closed this as completed Apr 17, 2024
@jdforsythe
Copy link
Author

jdforsythe commented Apr 18, 2024

@manast

The Reference says otherwise:

https://github.com/OptimalBits/bull/blob/develop/REFERENCE.md#jobgetstate

As does the presence of this function:

https://github.com/OptimalBits/bull/blob/develop/lib/job.js#L425-L429

As well as the getState() function:

https://github.com/OptimalBits/bull/blob/develop/lib/job.js#L435-L459

And this comment from the job tests:

bull/test/test_job.js

Lines 267 to 268 in 60fa88f

// This check is a bit of a hack. A job that is not found in any list will return the state
// stuck.

"A job that is not found in any list will return the state stuck"

And in any case, if 'stuck' wasn't a valid status, it should be removed from the types, not ignored.

https://github.com/OptimalBits/bull/blob/develop/index.d.ts#L250-L261

We cannot use JobStatus as the return type from getState():

Screenshot 2024-04-18 at 9 10 55 AM

We must use a union with 'stuck' every time to remove the type error:

Screenshot 2024-04-18 at 9 11 20 AM

@manast
Copy link
Member

manast commented Apr 18, 2024

I think some guy contributed to that and got merged somehow. It is not a state as far as I am concerned. Why do you need to care about this?

@jdforsythe
Copy link
Author

@manast because we have to keep adding it to our code to get the typing correct. We'd be fine if it were removed completely. We don't care about stuck but we're forced to add it to the type every time we get a job state.

I'd be happy to submit a PR to remove it, but what should getState() return if it's not in any of the lists?

@manast
Copy link
Member

manast commented Apr 19, 2024

A job must be in some list otherwise it would be a bug in bull, or the job has been removed and is not in the queue at all anymore.

@jdforsythe
Copy link
Author

@manast I submitted a PR to remove "stuck":

#2725

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

Successfully merging a pull request may close this issue.

2 participants