firebase / firebase-functions-test

MIT License
232 stars 48 forks source link

Incorrect "wrap" typing for v2 onCall cloud functions #163

Closed trex-quo closed 1 month ago

trex-quo commented 1 year ago

Version info

firebase-functions-test: 2.3.0

firebase-functions: 3.22.0

firebase-admin: 9.1.1

Test case

Live code:

import * as functions from "firebase-functions/v2";
export const myv2function = functions.https.onCall(
   { minInstances: 1 }, 
   async ({data, auth }) => {
      ...
   }
);

Note the above syntax differences from v1 onCall functions. There is no longer a pair of (data, context) parameters, only a single functions.https.CallableRequest<any> parameter. Additionally, the function options (e.g. minInstances) is a parameter instead of a sub-function of the functions class.

Test code:

import fftest from "firebase-functions-test";
const fft = fftest();

describe("test", () => {
   let functions;
   before(() => {
      functions = require("./myv2function.f");
   });

   it("My first test", async () => {
      const wrappedFunction = fft.wrap(functions.myv2function);
      let data = {};
      let result = await wrappedFunction({ data, auth: { uid: "uid" } });
   });
});

Steps to reproduce

Run the above sample with a //@ts-ignore above the await wrappedFunction(...) line. It works fine and seems like the wrapping for the new v2 functions works well. However, when removing the //@ts-ignore line, I get a type error. If I try to run the test code with the old v1 syntax (data, context) it does not work either.

Expected behavior

No type error when running the above code snippet, as the code is correct. v2 function overload signature needs to be added.

Actual behavior

Receive type error

Argument of type '{ data: {}; auth: { uid: string; }; }' is not assignable to parameter of type 'ContextOptions | undefined'.
  Object literal may only specify known properties, and 'data' does not exist in type 'ContextOptions'.ts(2345)
TheIronDev commented 1 year ago

@trex-quo Thank you for the bug report!

V2 Callable Function wrapping has not been implemented yet.

Theres going to need to be some work to implement this. I've added the "feature request" label and will follow up on this thread when we implement it.

Thank you again!!

trevor-rex commented 1 year ago

Hello, I wanted to check in on this. I see that 2nd gen firestore triggers have been added, but was wondering if onCall wrapping has been implemented yet? Having been following the v2 function development closely, it seems like onCalls would be covered ahead of firestore triggers

pehagg commented 1 year ago

Gen2 functions are GA now. Being unable to write tests for them is a showstopper for us to migrate to gen2. @TheIronDev , any chance you could bump the priority on this one?

cupidchan commented 1 year ago

I migrated all Functions to v2, then found wrap has not implemented yet. I think at least the documentation should warn the user NOT to upgrade to v2. @TheIronDev, Is there any timeline to release it?

trevor-rex commented 1 year ago

I migrated all Functions to v2, then found wrap has not implemented yet. I think at least the documentation should warn the user NOT to upgrade to v2. @TheIronDev, Is there any timeline to release it?

If it's helpful, it is still possible to use the existing library for wrapping on call functions. My team used some trial and error to find that you can invoke the wrapped function with its v2 arguments as so:

import ffTest from "firebase-functions-test";
const fft = ffTest();
...
const { function } = await import("../function.f");
// @ts-ignore - wrap doesn't support .onCall v2 yet
const wrapped = fft.wrap(function);
const data = {};
const result = await wrapped({ data, auth: {} });

You can see that if using typescript, you may need to add a ts-ignore statement since the proper types don't exist yet.

This does not work for firestore trigger functions, however. Those are entirely broken and untestable with this library.

TheIronDev commented 1 year ago

Howdy! @taeold would be the best person that could speak to timelines.

Cheers, Tyler

cupidchan commented 1 year ago

Thanks @trevor-rex! Since it's involved testing and the code quality, I prefer to use something that is "officially" supported. @taeold Do you know when will this be ready?

cupidchan commented 1 year ago

@TheIronDev, who else would know the timeline besides @taeold ?

Psycarlo commented 9 months ago

The documentation should clearly tell users NOT to migrate to v2. Tests are a big part of a maintanable project, and if this is not ready, the documentation should let us know.

Psycarlo commented 8 months ago

Any progress on this?

cupidchan commented 6 months ago

Happy 2024! And I would like to update my function to v2 as my resolution, if this issue is fixed. @TheIronDev @taeold Any update on this issue?

sebastianrueckerai commented 5 months ago

Are firebase V2 functions really not properly testable, still after 2 years ? Or is this issue fixed?

cupidchan commented 5 months ago

Looks like I am not the only one waiting for an answer for this long due question...

sebastianrueckerai commented 5 months ago

@TheIronDev is there a timeline to make trigger functions and onCall testable? It seems weird to promote functions V2 when testing them is not full supported.

sebastianrueckerai commented 5 months ago

Sorry, @taeold seems to be the right person to ask!

So, any chance to see this in 2024?

JakubKontra commented 5 months ago

Any chance?

cupidchan commented 5 months ago

Currently, @TheIronDev is the assignee. Should this issue be reassigned to @taeold or someone else to get a better attention to this issue?

Psycarlo commented 5 months ago

I still didn't migrate to v2 because of this.

inlined commented 5 months ago

Unassigning @TheIronDev as he no longer works on the product. Will put this on our internal tracker to prioritize

cupidchan commented 5 months ago

@inlined it's almost another month now. Do you have at least some ideas when will this be implemented? Encouraging people to move to v2, yet not supporting wrap doesn't make sense.

jackarens commented 5 months ago

We ran into this issue and were able to solve it with the following generic interface as a temporary workaround. Add this helper definition and use it when you're wrapping V2 callable functions.

/**
 * CallableV2Request is a helper interface to give typings to v2 cloud functions
 * when testing using firebase-functions-test. It is a stripped down version of
 * CallableRequest from firebase-functions/lib/common/providers/https.
 *
 * Example usage (in spec file):
 * import fftest from "firebase-functions-test";
 * const fft = fftest(*firebase config here*);
 * const wrappedFunction = fft.wrap<
 *   CallableV2Request<CallableFunctionDataInterface>
 * >(actualFunction as any);
 * */
export interface CallableV2Request<T = any> {
  data: T;
  auth?: {
    uid: string;
  };
}

Hope this helps while they finish updating this library.

cupidchan commented 5 months ago

Thanks @jackarens! Do you aware of any limitation or issue with this approach? @inlined will you recommend this as a fair workaround before the real solution is in place?

VOIDCRUSHER commented 4 months ago

Seconding @cupidchan on thanking @jackarens for the workaround. Worked perfectly for me for onCall function wrapping. I will note that while v2 triggers did not give a type error anymore, I still had to refactor all my tests to replace calls to test.makeObjectMetadata() and test.makeDocumentSnapshot() with mocked StorageEvents and FirestoreEvents. Hoping this gets bumped up in priority. The main developer documentation currently recommends switching over to gen2 but if you dig into the API reference, most interfaces are marked as beta, which makes complete firebase-functions-test support for v2 critical for adoption.

cupidchan commented 4 months ago

@inlined have you got a chance to review the workaround above? Also, do you know when will this be officially support? This issue has been opened for a while, it will be great if we have a timeline. Will that be you or someone else working on this?

inlined commented 4 months ago

I don't think merely coercing the type definitions will work here, because the library actually fills in dummy values.

We've gotten some more staffing for our various functions SDKs and I can put this third on our priority list (behind a new event type and a scheduled functions emulator). In the meantime, you can just call myCallableFunction.run(<args>) and provide dummy values for auth, instanceIdToken, etc if you rely on them.

cupidchan commented 4 months ago

Thanks @inlined do you have any rough timeline for this to be done, maybe by Google Next 2024 next month? 😄 BTW. I will be there in case you want to meet and greet!

inlined commented 4 months ago

Since all cloud functions have a run method that lets you invoke the function as a normal javascript function, this repo hasn't really been prioritized as much as people in this issue would like (while the interface is simple to use, it became a nightmare to build as we upgraded to typescript and again as we changed interfaces with v2). With GCP Next and I/O coming in short succession we're heads down on some exiting stuff and I don't foresee this coming until after.

Sadly, I won't be at GCP Next, though Firebase will have a presence there (including the GPM who oversees Functions). If you happen to be coming to I/O as well, I'll likely be there and even if I'm not can drive down to grab happy hour with our devs.

SupposedlySam commented 3 months ago

fyi, for anyone that's having trouble getting the work-around types to work properly - I ended up using this

import firebaseFunctionsTest from 'firebase-functions-test';
import { FeaturesList } from 'firebase-functions-test/lib/features';

...

type V2CallableReturn<T> = ReturnType<typeof onCall<T>>;

type WrapV2Function = <T>(
  cloudFunction: V2CallableReturn<T>
) => ReturnType<FeaturesList['wrap']>;

export type WrapV2Features = Omit<FeaturesList, 'wrap'> & {
  wrapV2: WrapV2Function;
};

export function _initializeFunctionsTest(): WrapV2Features {
  const { wrap, ...features } = firebaseFunctionsTest(
    // <your config here>
  );

  function wrapV2<T>(cloudFunction: V2CallableReturn<T>) {
    return wrap(cloudFunction as any);
  }

  return {
    ...features,
    wrapV2,
  };
}

Then elsewhere just call the wrapV2 function.

Let me know if I missed anything!

cupidchan commented 3 months ago

@SupposedlySam, how is this different from the one suggested by @jackarens, which looks much simpler?

fyi, for anyone that's having trouble getting the work-around types to work properly - I ended up using this

import firebaseFunctionsTest from 'firebase-functions-test';
import { FeaturesList } from 'firebase-functions-test/lib/features';

...

type V2CallableReturn<T> = ReturnType<typeof onCall<T>>;

type WrapV2Function = <T>(
  cloudFunction: V2CallableReturn<T>
) => ReturnType<FeaturesList['wrap']>;

export type WrapV2Features = Omit<FeaturesList, 'wrap'> & {
  wrapV2: WrapV2Function;
};

export function _initializeFunctionsTest(): WrapV2Features {
  const { wrap, ...features } = firebaseFunctionsTest(
    // <your config here>
  );

  function wrapV2<T>(cloudFunction: V2CallableReturn<T>) {
    return wrap(cloudFunction as any);
  }

  return {
    ...features,
    wrapV2,
  };
}

Then elsewhere just call the wrapV2 function.

Let me know if I missed anything!

SupposedlySam commented 3 months ago

Good question @cupidchan. The main difference is that I'm pulling the type directly from the onCall function instead of redeclaring it manually. Additionally, this provides the full implementation you would need vs just providing the interface itself. Is there a way to simplify what I have as the full implementation?

cupidchan commented 3 months ago

Thanks @SupposedlySam! It looks like it can be the "fix" than a workaround. @inlined, do you think you can adopt this approach in your solution?

Good question @cupidchan. The main difference is that I'm pulling the type directly from the onCall function instead of redeclaring it manually. Additionally, this provides the full implementation you would need vs just providing the interface itself. Is there a way to simplify what I have as the full implementation?

cupidchan commented 2 months ago

@exaby73 when do you think the PR will be in, as it's been a month already...

exaby73 commented 2 months ago

@cupidchan Hey. Sorry I've been preoccupied with some other tasks. I am planning on completing this this week :) It ready, just some typing that needs to be fixed

cupidchan commented 2 months ago

That's wonderful @exaby73 This long waiting feature is finally here!! Looking forward to the final merge of your PR!

sebastianrueckerai commented 1 month ago

Any news on this? @cupidchan A lot of people are excited for this after your announcement 2 weeks ago! :)

cupidchan commented 1 month ago

Any news on this? @cupidchan A lot of people are excited for this after your announcement 2 weeks ago! :)

@sebastianrueckerai, this question should actually address to @exaby73. I also got an email saying v2 will become the default version soon so we really need this fixed in PROD.

tpiaggio commented 3 weeks ago

Why is this issue closed? I haven't seen a solution yet?

cupidchan commented 3 weeks ago

@tpiaggio, I have just upgrade to the latest firebase and the wrapper is available for the v2 functions. I also upgrade my V1 to V2 along with tests. Please try and see if that works for you too.

tpiaggio commented 3 weeks ago

Yeah, I managed to make it work, thanks @cupidchan. The key for me was to use as any when passing parameters to the function, like this: const wrapped = test.wrap(myFunction); const response = await wrapped({} as any); Unfortunately, that part is not clear in the docs so I had to see the actual PR with the release to see the changes and that's when I saw it here.