Repository navigation
fix!: improve v2 callable typing - #328
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the TypeScript types for wrapping v2 callable Cloud Functions to support generic input and return types (T and Return), replacing the previous usage of any. It introduces a new WrappedV2CallableRequest<T> type to represent the wrapped request object. The reviewer feedback suggests several improvements to enhance usability and safety: making the data property optional when the input type T can be undefined, renaming the parameter of WrappedV2CallableFunction from data to request (and making it optional), and safely handling potential undefined request arguments at runtime within wrapV2 to prevent crashes.
# Conflicts: # CHANGELOG.md # src/main.ts
ajperel
left a comment
There was a problem hiding this comment.
Thanks for putting this together and tackling issue #257.
Stepping back and looking at the current type definition, and after a LOT of discussion with AI, I'd like to propose simplifying this to a permissive, single-optional-parameter model, mirroring how V2 CloudEvents (WrappedV2Function) works across the rest of this library.
The Problem with Strict Arity Enforcement
Right now, the PR uses TakesNoData<T> and tuple rest arity (...args: WrappedV2CallableArgs<T>) to conditionally require 1 argument unless T is strictly void or undefined. Because TakesNoData explicitly excludes any (0 extends 1 & T ? false : ...), it introduces noticeable friction for common unit testing patterns:
-
Untyped Callables (
onCall((req) => ...)Defaults toT = any):
In JavaScript and untyped TypeScript, callables default toT = any. BecauseTakesNoData<any>evaluates tofalse, developers cannot write:await wrapped(); // ❌ TS2554: Expected 1 arguments, but got 0 await wrapped({ auth: { uid: '123' } }); // ❌ TS2345: Property 'data' is missing
Instead, they are forced to pass boilerplate
{ data: undefined }on functions that don't take data. -
Early-Exit Auth & Permission Testing:
A common unit test for a callable is testing authorization guards:export const transferFunds = https.onCall<{ amount: number }>(async (req) => { if (!req.auth) throw new HttpsError('unauthenticated', 'Login required'); return processTransfer(req.data.amount); });
When testing that unauthenticated users are rejected, the test only cares about
auth:await expect(wrapped({ auth: undefined })).to.be.rejectedWith('unauthenticated');
Under strict arity, this fails to compile unless the developer invents dummy payload data (
{ auth: undefined, data: { amount: 0 } }), which adds noise and can mask regressions (e.g. if the auth guard is accidentally reordered). -
Optional Payload Unions (
onCall<string | undefined>):
[string | undefined] extends [undefined | void]evaluates tofalse, so optional payload functions are also forced to require{ data: ... }.
Consistency with V2 CloudEvents (WrappedV2Function)
In WrappedV2Function (src/v2.ts), the input parameter is completely optional:
export type WrappedV2Function<T extends CloudEvent<unknown>> = (
cloudEventPartial?: DeepPartial<T | object>
) => any | Promise<any>;V2 CloudEvents embraces the philosophy that in a unit testing SDK, permissiveness is key: developers specify only the partial slice of state relevant to the test, and the framework safely handles the rest.
At runtime, wrapV2 already does:
return cloudFunction.run({ data: undefined, ...request } as CallableRequest<any>);Because the runtime is already 100% crash-safe when called with zero arguments or without data, our TypeScript definitions should match that runtime flexibility.
Proposed Minimal Fix
We can drop TakesNoData, WrappedV2CallableArgs, and the DeepPartial<Required<Omit<...>>> workaround, replacing them with a flat, clean interface:
export type WrappedV2CallableRequest<T> = {
data?: T;
rawRequest?: Partial<Request>;
auth?: DeepPartial<CallableRequest['auth']>;
app?: DeepPartial<CallableRequest['app']>;
instanceIdToken?: string;
acceptsStreaming?: boolean;
};
/**
* A function that can be called with a {@link CallableRequest} for a v2 callable function.
* Every request field is optional to allow focused unit testing of auth, headers, or payloads.
*/
export type WrappedV2CallableFunction<T = any, Return = any> = (
request?: WrappedV2CallableRequest<T>
) => Return;Why this is better:
- Solves #257 completely: Passing
{ data: { whatever: "mydata" } }works cleanly,rawRequestacceptsPartial<Request>, and payload type checking ondatais still enforced if provided (e.g.{ data: { a: 'invalid' } }still fails). - First-class testing ergonomics:
wrapped(),wrapped({ auth: { uid } }), andwrapped({ rawRequest: { headers } })all compile and run naturally without ceremony. - IDE Tooltip Clarity: Hovering over
WrappedV2CallableFunctionin an editor shows a clean, readable signature with immediate autocompletion, rather than multi-line conditional intersection types. - Maintenance & Simplicity: Removes ~30 lines of conditional type gymnastics and compiler tricks (
0 extends 1 & T), making the code much easier to maintain asfirebase-functionsevolves.
In spec/v2.spec.ts, this would simply mean updating the untyped test to verify that wrappedCF() and wrappedCF({ auth: ... }) succeed without arguments (matching onCall<void>), rather than expecting a compile error.
What do you think about adopting this simpler, permissive approach?
|
@ajperel Thank you for taking the time to work on this! I definitely think going with this simpler/cleaner interface is better. There are three points that I think are worth raising w.r.t this approach, let me know your thoughts on them. 1.
|
In an appropriate test file. |
|
@ajperel Okay great, yeah I think in 2 this is the right tradeoff as well. I will make the changes for points 1 and 3. |
Use a flat request type so tests can omit data or the whole request, wrap auth and app in NonNullable so the published types compile under strict, and add a test that fails when CallableRequest gains a field.
ajperel
left a comment
There was a problem hiding this comment.
Nice. Thanks for iterating on this.
Fixes #257.
Calling a wrapped v2 callable from TypeScript required every
CallableRequestfield, so tests neededas anycasts and gotanyback.This keeps the callable's request and return types through
wrap()andwrapV2(), and makes every request field optional, includingdataand the request itself.datais a runtime error, not a compile error, so tests can pass onlyauthor headers. This matches how wrapped CloudEvent functions work.Tested with unit tests and a
--stricttype check of the published types against firebase-functions 4.9.0.