fix: allow dynamic strings as jobSchedulerId in upsertJobScheduler (#3937)#3942
fix: allow dynamic strings as jobSchedulerId in upsertJobScheduler (#3937)#3942Manmeet567 wants to merge 2 commits intotaskforcesh:masterfrom
Conversation
| @@ -0,0 +1,24 @@ | |||
| import { Queue } from '../src/classes/queue'; | |||
There was a problem hiding this comment.
of historical reasons we do not create files for specific issues, instead we add a new test on the proper test file that covers job schedulers functionality.
There was a problem hiding this comment.
Pull request overview
Updates the Queue.upsertJobScheduler TypeScript overloads to decouple the scheduler’s identifier from the queue’s NameType, enabling dynamic string IDs (e.g., UUIDs) while keeping job names strictly typed.
Changes:
- Added an overload that accepts
jobSchedulerId: stringwhen ajobTemplate.name: NameTypeis provided. - Updated the internal call to
JobScheduler.upsertJobSchedulerto accommodatejobSchedulerId: string. - Added a new test file intended to reproduce issue #3937.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/classes/queue.ts |
Expands upsertJobScheduler overloads to allow dynamic string scheduler IDs when the job name is provided separately. |
tests/test_issue_3937.test.ts |
Adds a reproduction file for #3937, but currently executes side effects at import time and doesn’t actually enforce TS type-checking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const queue = new Queue<JobData, void, JobName>('issue-3937'); | ||
| const dynamicId = String('inst_12345'); | ||
|
|
||
| // Scheduler IDs must accept dynamic strings independently of the job name type. | ||
| queue.upsertJobScheduler( | ||
| dynamicId, |
There was a problem hiding this comment.
This test file executes queue.upsertJobScheduler(...) at module top-level without awaiting the promise or cleaning up the Queue/Redis connection. In Vitest this will run during file import, can create open handles (hanging the run) and can trigger unhandled promise rejections. Please wrap this in an it(...) block (or an if (false) type-check-only block) and ensure the queue is closed/cleaned up if the call is actually executed.
| @@ -0,0 +1,24 @@ | |||
| import { Queue } from '../src/classes/queue'; | |||
There was a problem hiding this comment.
Most tests import from the barrel export ../src/classes rather than deep-importing specific class files. Consider switching to import { Queue } from '../src/classes' to match the existing test conventions and avoid differences between entrypoints.
| import { Queue } from '../src/classes/queue'; | |
| import { Queue } from '../src/classes'; |
| import { Queue } from '../src/classes/queue'; | ||
|
|
||
| type JobName = 'generate-report' | 'send-email'; |
There was a problem hiding this comment.
This file won’t actually protect against TypeScript regressions as written: tsconfig.json excludes tests/* from tsc, and Vitest transpiles TS without type-checking by default. To make this a real reproduction for #3937, consider adding a dedicated typecheck step for tests (e.g. vitest --typecheck or a tsc -p tsconfig.test.json script) or moving this into whatever type-test mechanism the repo uses.
This PR resolves issue #3937, where the jobSchedulerId parameter in the upsertJobScheduler method was strictly constrained to the NameType generic.
In many real-world use cases, while the JobName might be part of a strict union (e.g., 'report' | 'email'), the jobSchedulerId needs to be a unique, dynamic identifier such as a UUID or a database ID. The previous type definition forced these IDs to match the NameType union, causing TypeScript errors when using non-literal strings.
Changes
src/classes/queue.ts: Updated the upsertJobScheduler method overloads to accept string for the jobSchedulerId parameter.
src/classes/job-scheduler.ts: (If applicable) Aligned the internal scheduler types to support general string IDs.
Maintained existing generics for DataType, ResultType, and NameType to ensure that job data and return types remain strictly typed.
Verification & Testing
Automated Test: Added a reproduction test case in src/tests/test_issue_3937.ts.
Scenario: Verified that a Queue defined with strict JobNames successfully accepts a dynamic string variable as a jobSchedulerId without TypeScript compilation errors.
Build: Confirmed that yarn build and yarn lint pass successfully on a local environment.
Closes #3937