-
Notifications
You must be signed in to change notification settings - Fork 895
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
Support streaming streaming responses for callable functions. #8609
Conversation
🦋 Changeset detectedLatest commit: 9ea463d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
…e-js-sdk into dl-fn-streaming
Changeset File Check ✅
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI stuff that you want to do before you finalize and merge:
(1) To address the formatting CI failure, run yarn format
locally.
(2) To address the docgen failure, run yarn docgen:all
locally. This will modify some files in docs-devsite
which will require a techwriter review approval in addition to a JS core team review. go/firebase-contacts tells me the techwriter assigned to CF3 is Eric Gilmore.
@@ -37,7 +37,8 @@ | |||
"test:browser": "karma start", | |||
"test:browser:debug": "karma start --browsers=Chrome --auto-watch", | |||
"test:node": "TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'src/{,!(browser)/**/}*.test.ts' --file src/index.ts --config ../../config/mocharc.node.js", | |||
"test:emulator": "env FIREBASE_FUNCTIONS_EMULATOR_ORIGIN=http://localhost:5005 run-p --npm-path npm test:node", | |||
"test:emulator": "env FIREBASE_FUNCTIONS_EMULATOR_ORIGIN=http://127.0.0.1:5005 run-p test:node", | |||
"test:emulator": "env FIREBASE_FUNCTIONS_EMULATOR_ORIGIN=http://127.0.0.1:5005 run-p --npm-path npm test:node", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is my github diff wonky or are these two lines with the same script name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops i think this is an artifact of a merge gone wrong. Reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this change to use 127.0.0.1
instead of localhost
still be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my dev machine, only 127.0.0.1 works (due to changes in recent Mac version that resolves localhost to ipv6), but I can revert the change if preferred.
@@ -37,7 +37,8 @@ | |||
"test:browser": "karma start", | |||
"test:browser:debug": "karma start --browsers=Chrome --auto-watch", | |||
"test:node": "TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'src/{,!(browser)/**/}*.test.ts' --file src/index.ts --config ../../config/mocharc.node.js", | |||
"test:emulator": "env FIREBASE_FUNCTIONS_EMULATOR_ORIGIN=http://localhost:5005 run-p --npm-path npm test:node", | |||
"test:emulator": "env FIREBASE_FUNCTIONS_EMULATOR_ORIGIN=http://127.0.0.1:5005 run-p test:node", | |||
"test:emulator": "env FIREBASE_FUNCTIONS_EMULATOR_ORIGIN=http://127.0.0.1:5005 run-p --npm-path npm test:node", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this change to use 127.0.0.1
instead of localhost
still be here?
…e-js-sdk into dl-fn-streaming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few thoughts and tweaks, thanks!
https://github.com/firebase/firebase-js-sdk | ||
{% endcomment %} | ||
|
||
# HttpsCallable interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm puzzled about why this is new/green/add when it seems to have existed already in line 43 of public_types.ts, iiuc.
In any case, it would be great to have clarity on whether we really mean "Google Cloud Functions" or whether we should stick with just "Cloud Functions" or even "Cloud Functions for Firebase."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a while since we made significant changes to this code, and it's possible the these type definition preceeds the devsite doc generation process I'm running here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll stick with Cloud Functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG, thanks!
https://github.com/firebase/firebase-js-sdk | ||
{% endcomment %} | ||
|
||
# HttpsCallable interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG, thanks!
The new
.stream()
API allows the client to consume streaming responses from the WIP streaming callable functions in Firebase Functions Node.js SDK.When client makes a request to the callable function w/ header
Accept: text/event-stream
, the callable function responds with response chunks in Server-Sent Event format.The sdk changes here abstracts over the wire-protocol by parsing the response chunks and returning an instance of a
AsyncIterable
to consume to data: