feat(functions): Add features to task queue functions - #2216
Conversation
Sorry, something went wrong.
| * The default is 10 minutes. The deadline must be in the range of 15 seconds and 30 minutes. | ||
| */ | ||
| dispatchDeadlineSeconds?: number; | ||
| /** |
There was a problem hiding this comment.
nit: newline between commented fields (e.g. here and headers)
Sorry, something went wrong.
| it(`should throw if options is ${invalidOption}`, () => { | ||
| expect(() => apiClient.enqueue({}, FUNCTION_NAME, '', invalidOption as any)) | ||
| .to.throw('TaskOptions must be a non-null object'); | ||
| expect(apiClient.enqueue({}, FUNCTION_NAME, '', invalidOption as any)) |
There was a problem hiding this comment.
Here and elsewhere, you need to make the test case async and then await this expectation or no check will actually be performed
Sorry, something went wrong.
There was a problem hiding this comment.
Hm I deliberately chose to use chai's eventually to wait on the Promise returned by enqueue calls instead of async/await, to align better with the existing implementation which generally avoids async/await. Should I go ahead and refactor the unit tests to use async/await instead?
Sorry, something went wrong.
There was a problem hiding this comment.
Since we don't make any changes to the package.json in this PR, let's revert the lock file changes and let dependabot take care of the package-lock.json updates.
Sorry, something went wrong.
lahirumaramba
left a comment
There was a problem hiding this comment.
Thank you for the PR!
Please get a TW review on the documentation changes before merging this.
Sorry, something went wrong.
egilmorez
left a comment
There was a problem hiding this comment.
I got a certain way through this and GH told me my diff was outdated . . . submitting these comments so far and will look again, thanks!
Sorry, something went wrong.
| * If not provided, one will be automatically generated. | ||
| * If provided explicitly specifying a task ID enables task de-duplication. If a task's ID is | ||
| * identical to that of an existing task or a task that was deleted or executed recently then | ||
| * the call will throw a TaskAlreadyExists error. Another task with the same id can't be |
There was a problem hiding this comment.
Suggest TaskAlreadyExists and capped "ID" in the next line.
Sorry, something went wrong.
There was a problem hiding this comment.
Actually wanted to get your thoughts on how to document this — instead of throwing a TaskAlreadyExists error, we changed the behavior to throw a FirebaseFunctionsError with code "functions/task-already-exists". So if a user wanted to check if a thrown error is due to the task already existing, they would check like so:
try {
await queue.enqueue(...);
} catch (err: unknown) {
if (err.code === "functions/task-already-exists") {
...
}
}
Is this comment in the functions API sufficient to document this behavior?
Sorry, something went wrong.
lahirumaramba
left a comment
There was a problem hiding this comment.
LGTM! Thank you!
Sorry, something went wrong.
Since CF3 released support for Task Queue Functions at Google I/O 2022, users have made several feature requests. This PR enhances the Task Queue functions API to enable the following:
See firebase/firebase-functions#1423 for the sister changes in the functions SDK.
RELEASE NOTES: Headers passed to TaskQueue HTTPS handlers are now available in the context/event. Added the ability to name the tasks by including an id when enqueueing tasks. Added the ability to delete an enqueued task if it has not yet completed.