Skip to content

Use conditional typing for defining the right set of options #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

phcoliveira
Copy link
Contributor

@phcoliveira phcoliveira commented Aug 31, 2022

Description

This PR fixes the typing of a wrapped function's second argument, namely: options. Such options are described by two types: CallableContextOptions and EventContextOptions. The first is only due to the http.onCall(), since the function http.onRequest() is not supposed to be wrapped. The latter is due to all other cloud functions under V1.

With the current implementation, the type of the options is the result of a union of both types mentioned above. Because of that, a consumer of this API was led to believe that some options due to a "event" function were available to "callable" function, and vice-versa.

This PR fixes that by conditionally assigning the type of such options depending on the function being wrapped.

Code sample

There is no API change. But here is an example of what this PR introduces:

import * as functions from 'firebase-functions'
import firebaseFunctionsTest from 'firebase-functions-test'

const testFunctions = firebaseFunctionsTest()

const httpsOnCall = functions.https.onCall(/* ... */)
const wrappedHttpsOnCall = testFunctions.wrap(httpsOnCall, { /* CallableContextOptions */ })

const databaseOnCreate = functions.database.onCreate(/* ... */)
const wrappedDatabaseOnCreate = testFunctions.wrap(databaseOnCreate, { /* EventContextOptions */ })

@google-cla
Copy link

google-cla bot commented Aug 31, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@phcoliveira phcoliveira changed the title Use conditionally typing for defining the right set of options Use conditional typing for defining the right set of options Sep 1, 2022
@TheIronDev
Copy link
Contributor

@phcoliveira Thanks for opening up a pull request! The code makes sense to me.

I triggered the workflow actions, but it looks like you still need to sign the CLA.

@phcoliveira
Copy link
Contributor Author

@phcoliveira Thanks for opening up a pull request! The code makes sense to me.

I triggered the workflow actions, but it looks like you still need to sign the CLA.

Good to hear, @TheIronDev. I will check this CLA thing in the morning. Bedtime for me.

@phcoliveira phcoliveira changed the title Use conditional typing for defining the right set of options Use conditional typing for defining the right set of option Sep 2, 2022
@phcoliveira phcoliveira changed the title Use conditional typing for defining the right set of option Use conditional typing for defining the right set of options Sep 2, 2022
@phcoliveira
Copy link
Contributor Author

I signed the CLA, @TheIronDev.

@TheIronDev
Copy link
Contributor

TheIronDev commented Sep 2, 2022

@phcoliveira

This is definitely an improvement!! Thank you very much for the pull request!

We don't cut new releases on Fridays, but I'll follow up with publishing a patch on Tuesday! (the next business day)

Thank you again!
Tyler

@TheIronDev TheIronDev merged commit 92d7be3 into firebase:master Sep 2, 2022
@phcoliveira
Copy link
Contributor Author

Happy to help, @TheIronDev.
Closes #162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants