Job.fromId String Parameter Contradicts Method Comment/Implementation

[bobdercole]

The jobId parameter in the following method accepts string. However, the comment in the method states that it could be undefined. The conditional then proceeds to check for a truthy value.

bullmq/src/classes/job.ts

Lines 138 to 145 in 36726bf

	static async fromId(queue: QueueBase, jobId: string) {
	// jobId can be undefined if moveJob returns undefined
	if (jobId) {
	const client = await queue.client;
	const jobData = await client.hgetall(queue.toKey(jobId));
	return isEmpty(jobData) ? null : Job.fromJSON(queue, jobData, jobId);
	}
	}

This was brought up while trying to fix the return type of the method in #133. I believe we came to the conclusion to remove the conditional check.

We would also need to review any code that consumes Job.fromId to ensure undefined isn't being passed from anywhere. These could be easily found by switching strictNullChecks to true. I believe this was brought up in GH-112.

bullmq/tsconfig.json

Line 16 in 36726bf

"strictNullChecks": false,

An alternative is to simply change the parameter to jobId?: string. However, I feel like it's weird to have an optional ID parameter on a method named fromId.

I could make a PR once a merge decision is made for PR #133, since the code is sort of intertwined.

References:
#133 (comment)
#130 (comment)

[manast]

I agree that a method that does nothing if you do not provide the second parameter is very awkward. These are remains from older code, thats why it looks like this. So best is to keep it mandatory, but as you wrote we need to make sure that all the calls to this method always pass a defined jobId.