feat(functions): httpsCallable.stream support#8799
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mikehardy
left a comment
There was a problem hiding this comment.
None of my comments would result in any change in functionality - it all seems like it would work fine. Just trivial nitpicks or similar, along with notes on cross-checking performed (with positive outcomes)
So +1 and I'd squash merge - but leaving open in case you want to adopt any of the suggestions?
| s.private_header_files = "ios/**/*.h" | ||
| s.exclude_files = 'ios/generated/RCTThirdPartyComponentsProvider.*', 'ios/generated/RCTAppDependencyProvider.*', 'ios/generated/RCTModuleProviders.*', 'ios/generated/RCTModulesConformingToProtocolsProvider.*', 'ios/generated/RCTUnstableModulesRequiringMainQueueSetupProvider.*' | ||
| # Turbo modules require these compiler flags | ||
| s.compiler_flags = '-DFOLLY_NO_CONFIG -DFOLLY_MOBILE=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_CFG_NO_COROUTINES=1' |
There was a problem hiding this comment.
I believe the compiler_flags is already handled by the install_module_dependencies() call (which is...frankly non-negotiable so should be taken out of the if(defined?(install_modules_dependencies()) conditional and be the only path)
suggest experiment with removing this line and removing the conditional so that install_modules_dependencies is always called, and I think TurboModules will be set up correctly for all supported react-native
There was a problem hiding this comment.
I've made install_module_dependencies () always run.
I've tried a number of different ways not setting all headers private (nuclear option) but no matter what, the Swift module build blows up when it encounters C++ stdlib header from codegen.
| /// Swift wrapper for Firebase Functions streaming that's accessible from Objective-C | ||
| /// This is necessary because Firebase's streaming API uses Swift's AsyncStream which | ||
| /// doesn't have Objective-C bridging | ||
| @available(iOS 15.0, macOS 12.0, *) |
There was a problem hiding this comment.
this @available annotation shouldn't be necessary unless the pbxproj IPHONEOS_DEPLOYMENT_TARGET is wrong
In fact no, it is set to 10 ! That's not correct
4 places, here is an example
confirmed iOS 15 is already our minimum supported iOS version - we're issuing the merge result of this as a breaking change but it's related to functions going new architecture only, this iOS version doesn't have to be mentioned.
As a separate issue / peeled from this review (right-click on this comment and create issue - I formatted it well for that), we should raise the deploy target to 15 in the package pbxproj's to match current requirements
There was a problem hiding this comment.
Ok, if I understood you correctly - raise min deployment target in Functions package (and possibly across the board) in a separate PR(s)?
There was a problem hiding this comment.
yep exactly that - just peel this to another issue directly via the comment tools and table it since it's good for hygiene but that's low priority and this PR needs to go in
| streamOptions?: HttpsCallableStreamOptions, | ||
| ) => { | ||
| const platformOptions = | ||
| isAndroid || isIOS |
There was a problem hiding this comment.
could also be written maybe more concisely as !isOther but isOther is defined as !isAndroid && !isIOS so means literally the same thing. Philosophically I think isOther is technically more accurate as perhaps we add more native platforms that need the non-web type, but isOther is specifically how we say "firebase-js-sdk implementation" so is maybe best?
| isAndroid || isIOS | |
| !isOther |
| @"listenerId" : listenerIdNumber, | ||
| @"body" : event | ||
| }; | ||
| [[RNFBRCTEventEmitter shared] sendEventWithName:@"functions_streaming_event" |
There was a problem hiding this comment.
I have noticed event batching behavior on iOS only, and I trace it to this method call (so far).
Specifically if I add an NSLog call here before the RNFBRCTEventEmitter call, and I alter the Basic Stream local-test to wait 5 seconds, I get no events emitted for 20 seconds, then the first 4 come all at once. Then nothing, then the last event and the terminate event (you can see it in the timestamps):
The function itself (running in the emulator) emits logs as expected, every 5 seconds when configured for a 5 second delay.
I haven't determined why yet but it's worth noting as I believe batching these is unwanted, and android doesn't exhibit this behavior.
It could be in the functions emulator batching the chunk sends to iOS for some reason (but...the native/emulator event emitting doesn't do that on android?) or it could be in firebase-ios-sdk, or it could be in our native code / firebase-ios-sdk setup / callbacks somehow.
There was a problem hiding this comment.
Can't see anything on our iOS setup that would batch the responses to JS side. We can infer from android working fine, it isn't a problem with JS internal implementation. Sounds like a firebase-ios-sdk issue
| for try await response in stream { | ||
| switch response { | ||
| case .message(let message): | ||
| eventCallback([ |
There was a problem hiding this comment.
related to investigating why events are delayed/batched on iOS, they are also batched here - this isn't called every time the emulator function actually sends a chunk
|
@mikehardy - I think the build issue is related to changes made to podspec from PR review. I've reran debug build to confirm. Keeping my eye on it 👍 |
|
@mikehardy - failed. I think sendable protocol is related to Swift v6 that causes issue perhaps 🤔 |
|
swift to 5 did it, ios debug was a simulator pre-boot flake, re-running |
|
is stream() available now in main? |
|
@ftaibi yes on main and I believe if you look at the PR where it went in, you should be able to grab patch-package packages that apply to the current release (and bring it up to main+that PR at that point in time) if you would like to use it pre-release. |

Description
@russellwheatley taken over:
HttpsCallableStreamOptionswill only apply to web streaming. UseHttpsCallableOptionsfor iOS and android http rest calls/streaming.v.81.TaskExecutorServiceconstructor public on Firebase App so we can still use it.Related issues
fixes #8210
Release Summary
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter