Skip to content
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

Add httpsCallableFromURL #6162

Merged
merged 9 commits into from
Apr 19, 2022
Merged

Add httpsCallableFromURL #6162

merged 9 commits into from
Apr 19, 2022

Conversation

inlined
Copy link
Member

@inlined inlined commented Apr 16, 2022

Sorry for a late-coming urgent change; AFAICT nobody on the SDK team could staff these changes, so I'm trying to write the change for the top 3 SDKs before I/O freeze.

For context, see go/cf3v2-crash-landing.
For API approval, see go/callable-functions-custom-urls

@changeset-bot
Copy link

changeset-bot bot commented Apr 16, 2022

🦋 Changeset detected

Latest commit: 24e82d0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/functions-compat Patch
@firebase/functions Patch
firebase Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@inlined inlined requested a review from egilmorez as a code owner April 16, 2022 17:25
@github-actions
Copy link
Contributor

github-actions bot commented Apr 16, 2022

Changeset File Check ✅

  • No modified packages are missing from the changeset file.
  • No changeset formatting errors detected.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 16, 2022

Size Report 1

Affected Products

  • @firebase/functions

    TypeBase (38da5d9)Merge (852f765)Diff
    browser8.99 kB9.39 kB+397 B (+4.4%)
    esm511.1 kB11.5 kB+414 B (+3.7%)
    main11.8 kB12.4 kB+529 B (+4.5%)
    module8.99 kB9.39 kB+397 B (+4.4%)
  • @firebase/functions-compat

    TypeBase (38da5d9)Merge (852f765)Diff
    browser1.68 kB1.79 kB+111 B (+6.6%)
    esm51.83 kB1.98 kB+148 B (+8.1%)
    main2.20 kB2.34 kB+137 B (+6.2%)
    module1.68 kB1.79 kB+111 B (+6.6%)
  • bundle

    TypeBase (38da5d9)Merge (852f765)Diff
    functions (call)27.6 kB27.6 kB+40 B (+0.1%)
  • firebase

    TypeBase (38da5d9)Merge (852f765)Diff
    firebase-compat.js779 kB779 kB+179 B (+0.0%)
    firebase-functions-compat.js7.95 kB8.12 kB+169 B (+2.1%)
    firebase-functions.js31.1 kB32.0 kB+947 B (+3.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/3gHFrqXHAY.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 16, 2022

Size Analysis Report 1

Affected Products

  • @firebase/functions

    • httpsCallable

      Size

      TypeBase (38da5d9)Merge (852f765)Diff
      size5.98 kB6.02 kB+38 B (+0.6%)
      size-with-ext-deps21.1 kB21.1 kB+40 B (+0.2%)

      Dependency

      TypeBase (38da5d9)Merge (852f765)Diff
      functions

      11 dependencies

      _errorForResponse
      call
      codeForHTTPStatus
      decode
      encode
      failAfter
      httpsCallable
      httpsCallable$1
      mapValues
      postJSON
      registerFunctions

      12 dependencies

      _errorForResponse
      call
      callAtURL
      codeForHTTPStatus
      decode
      encode
      failAfter
      httpsCallable
      httpsCallable$1
      mapValues
      postJSON
      registerFunctions

      + callAtURL

    • httpsCallableFromURL

      Size

      TypeBase (38da5d9)Merge (852f765)Diff
      size?5.97 kB? (?)
      size-with-ext-deps?21.1 kB? (?)

      Dependency

      TypeBase (38da5d9)Merge (852f765)Diff
      functions?

      11 dependencies

      _errorForResponse
      callAtURL
      codeForHTTPStatus
      decode
      encode
      failAfter
      httpsCallableFromURL
      httpsCallableFromURL$1
      mapValues
      postJSON
      registerFunctions

      ?
      classes?

      ContextProvider
      FunctionsError
      FunctionsService

      ?
      variables?

      APP_CHECK_INTERNAL_NAME
      AUTH_INTERNAL_NAME
      DEFAULT_REGION
      FUNCTIONS_TYPE
      LONG_TYPE
      MESSAGING_INTERNAL_NAME
      UNSIGNED_LONG_TYPE
      errorCodeMap
      name
      version

      ?
      enums??

      External Dependency

      ModuleBase (38da5d9)Merge (852f765)Diff
      @firebase/app?

      _registerComponent
      registerVersion

      ?
      @firebase/component?

      Component

      ?
      @firebase/util?

      FirebaseError
      getModularInstance

      ?

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/oMe5L3sPEF.html

@google-oss-bot google-oss-bot added the doc-changes PRs that affect docs label Apr 16, 2022
Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, just a few small changes.

---
"@firebase/functions-compat": patch
"@firebase/functions": patch
"firebase-size-analysis": patch
Copy link
Contributor

Choose a reason for hiding this comment

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

firebase-size-analysis can be removed, it's in the ignored changesets list. Actually, the change to that file seems unneeded (see below) so also for that reason.

@@ -118,11 +118,28 @@ async function authLogout(app) {
*/
async function callFunctions(app) {
console.log('[FUNCTIONS] start');
const functions = getFunctions(app);
const callTest = httpsCallable(functions, 'callTest');
let functions = getFunctions(app);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is functions reassigned?

@@ -90,3 +91,23 @@ export function httpsCallable<RequestData = unknown, ResponseData = unknown>(
options
);
}

/**
* Returns a reference to the callable HTTPS trigger with the given name.
Copy link
Contributor

Choose a reason for hiding this comment

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

"given name" => "specified URL"?

@@ -22,11 +22,12 @@
"path": "functions",
"imports": [
"getFunctions",
"httpsCallable"
"httpsCallable",
"httpsCallableFromURL"
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't need to be added here, this represents a typical bundle of imports for a use case and it's not a common case that a developer would use httpsCallable and httpsCallableFromURL in the same app. In any case, since httpsCallable wraps the core code of httpsCallableFromURL, the bundle size should be about the same.

@@ -29,4 +29,4 @@
}
]
}
]
]
Copy link
Contributor

Choose a reason for hiding this comment

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

It still thinks there's a whitespace change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dunno why my formatter keeps on insisting on this. Edited in vim to avoid stripping the newline.

@@ -122,7 +122,24 @@ async function callFunctions(app) {
const callTest = httpsCallable(functions, 'callTest');
Copy link
Contributor

Choose a reason for hiding this comment

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

functions isn't reassigned but it looks like callTest is, and does still need the let.

Copy link
Member Author

Choose a reason for hiding this comment

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

lol. duh

Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

LG, thanks!

@inlined inlined merged commit c69c689 into master Apr 19, 2022
@inlined inlined deleted the inlined.callable-url branch April 19, 2022 20:32
@google-oss-bot google-oss-bot mentioned this pull request Apr 27, 2022
@firebase firebase locked and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
doc-changes PRs that affect docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants