Skip to content

Cache Functions instances by app #8258

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

Merged
merged 19 commits into from
Jun 16, 2021
Merged

Cache Functions instances by app #8258

merged 19 commits into from
Jun 16, 2021

Conversation

morganchen12
Copy link
Contributor

Fixes #7783.

@google-cla google-cla bot added the cla: yes label Jun 14, 2021
@google-oss-bot
Copy link

google-oss-bot commented Jun 14, 2021

Coverage Report

Affected SDKs

  • FirebaseFunctions-iOS-FirebaseFunctions.framework

    SDK overall coverage changed from 52.17% (8dd5fdd) to 60.63% (36bcbf3) by +8.46%.

    Filename Base (8dd5fdd) Head (36bcbf3) Diff
    FIRFunctions.m 54.10% 70.03% +15.93%
    FIRFunctionsComponent.m ? 86.30% ?

Test Logs

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the incremental coverage checker correct with its four reports? cc: @granluo

@paulb777 paulb777 requested a review from ryanwilson June 14, 2021 23:33
@granluo
Copy link
Contributor

granluo commented Jun 15, 2021

Is the incremental coverage checker correct with its four reports? cc: @granluo

For the latest reports, I double checked with the xcresult artifact in this run. The reports should be correct.

@morganchen12
Copy link
Contributor Author

Talked to Ryan offline and we agreed it'd be best to move Functions to broader FIRComponent adoption, with these specific points:

  • Functions should attempt to minimize creation of new instances as much as possible
  • Functions should return a new instance per combination of app, region, and custom domain

It's a bit late for code freeze, but we can punt to M99 if necessary.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on green

@morganchen12 morganchen12 merged commit 27faa28 into master Jun 16, 2021
@morganchen12 morganchen12 deleted the mc/emu branch June 16, 2021 04:45
@morganchen12 morganchen12 added this to the 8.2.0 - M98 milestone Jun 17, 2021
@firebase firebase locked and limited conversation to collaborators Jul 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Callable function returns 'Unauthenticated' error when using the functions emulator
5 participants