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

Firebase Functions ships with a binary #1003

Merged
merged 70 commits into from
Feb 2, 2022
Merged

Firebase Functions ships with a binary #1003

merged 70 commits into from
Feb 2, 2022

Conversation

taeold
Copy link
Contributor

@taeold taeold commented Nov 9, 2021

We introduce a bin associated with the firebase-functions SDK! The binary exposes a simple server on localhost that describes the Firebase Functions defined in the current directory:

$ STACK_CONTROL_API_PORT=8181 npx firebase-functions  # Optionally can pass in a FUNCTIONS_DIR as first arg 
Serving at port 8181

$ curl localhost:8181/__/stack.yaml | jq
{
  "endpoints": {
    "onreqv2": {
      "platform": "gcfv2",
      "labels": {},
      "httpsTrigger": {},
      "entryPoint": "onreqv2"
    },
    "onRequest": {
      "platform": "gcfv1",
      "httpsTrigger": {},
      "entryPoint": "onRequest"
    }
  },
  "specVersion": "v1alpha1",
  "requiredAPIs": []
}

@google-cla google-cla bot added the cla: yes label Nov 9, 2021
@taeold taeold changed the base branch from dl-container-contract to dl-endpoint-prop November 9, 2021 23:16
@taeold taeold requested a review from inlined November 9, 2021 23:30
Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

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

I like it, but let's get the port/Env var + the requiredAPIs stuff sorted out.

All in all though, this is much better than I could have done myself. We're really lucky that you've got experience with ESM modules vs pacakges.

spec/fixtures/sources/commonjs-grouped/g1.js Outdated Show resolved Hide resolved
spec/fixtures/sources/commonjs-grouped/index.js Outdated Show resolved Hide resolved
spec/runtime/loader.spec.ts Outdated Show resolved Hide resolved
src/bin/firebase-functions.ts Outdated Show resolved Hide resolved
src/bin/firebase-functions.ts Outdated Show resolved Hide resolved
src/bin/firebase-functions.ts Show resolved Hide resolved
src/runtime/loader.ts Outdated Show resolved Hide resolved
src/runtime/loader.ts Outdated Show resolved Hide resolved
taeold and others added 4 commits December 9, 2021 21:18
Follows up #999 to annotate each funuctions with `__endpoint` property.

Highlight of changes:

* Extend unit test coverage for all v1 providers
* Add `__endpoint` annotation to v1 task queue functions
* Add `__requiredAPIs` annotation to task queue and scheduler functions
* Explicitly set `__endpoint` to undefined in the handler namespace
* No SDK-level label setting in the __endpoint annotation.
@taeold taeold changed the base branch from dl-endpoint-prop to dl-container-contract December 10, 2021 05:39
@taeold taeold marked this pull request as ready for review December 10, 2021 19:29
@taeold taeold requested a review from inlined December 14, 2021 06:46
Copy link
Contributor

@colerogers colerogers left a comment

Choose a reason for hiding this comment

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

looks good, just one thing that I had a question on

src/runtime/manifest.ts Show resolved Hide resolved
Base automatically changed from dl-container-contract to master January 19, 2022 18:18
Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

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

Still looking good.

spec/runtime/loader.spec.ts Outdated Show resolved Hide resolved
spec/v1/providers/analytics.spec.ts Outdated Show resolved Hide resolved
src/bin/firebase-functions.ts Outdated Show resolved Hide resolved
src/bin/firebase-functions.ts Outdated Show resolved Hide resolved
res.setHeader('content-type', 'text/yaml');
res.send(JSON.stringify(backend));
res.send(JSON.stringify(stack));
Copy link
Member

Choose a reason for hiding this comment

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

I mean.... technically JSON is YAML?

{
api: 'cloudscheduler.googleapis.com',
reason: 'Needed for v1 scheduled functions.',
Copy link
Member

Choose a reason for hiding this comment

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

here's a case where I could argue for using "v1" because v2 will not use Pub/Sub.

Alternatively you can just remove this required API because it's part of the APIs that get enabled with Cloud Functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think Cloud Scheduler gets enabled as part of GCF, but I'm not sure what enables it since I do see it enabled when creating a new GCP project 🤔 . I'm going to add 'v1' to err on side of being safe, but can remove later when I dig in and figure out if we can rely on the cloud scheduler API being enabled on users project by some means.

@taeold taeold merged commit f3ed261 into master Feb 2, 2022
@taeold taeold deleted the dl-ff-runtime branch February 2, 2022 13:21
taeold added a commit that referenced this pull request Feb 7, 2022
…#1033)

Follows up #1032 for V2 API to fix #1031.

While rebasing the branch that moved the location of manifest definition (#1003), the original file was not removed and cleaned up. Having long-lasting PR is dangeroud 🤦🏼‍♂️ .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants