feat(functions): httpsCallable.stream support #8799
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mikehardy
changed the title
| // Ensure NativeRNFBTurboFunctions is registered - it should be registered by namespaced.ts | ||
| // but we verify and add removeFunctionsStreaming if needed | ||
| try { | ||
| const module = getReactNativeModule('NativeRNFBTurboFunctions'); | ||
| if (module && !module.removeFunctionsStreaming) { | ||
| module.removeFunctionsStreaming = () => {}; | ||
| } | ||
| } catch (_e) { | ||
| // Module not registered yet - register it ourselves as fallback | ||
| // This shouldn't happen if namespaced.ts imported correctly | ||
| setReactNativeModule('NativeRNFBTurboFunctions', { |
There was a problem hiding this comment.
This reads like a workaround - if there is a "should" and we're in a test, then it seems better to transform the expectation into a more concrete "must" and verify it or fail, then fix any underlying issue so the "must" level of API / function registration contract is met
workarounds in tests are a red flag because if we have to workaround in our own tests, what are library consumers going to have to do ?
| HttpsCallableReference httpReference = | ||
| functionsInstance.getHttpsCallableFromUrl(parsedUrl); |
There was a problem hiding this comment.
is there any required functional difference between this method from an Url and the above method except the setup before to get the HttpsCallableReference? It doesn't seem like there is much of a difference except augmented logging which the non-Url method could probably benefit from anyway
perhaps an extract-method refactor and then have both the Url version and non-Url version call a single implementation of all the subscription / event emit etc logic ?
| } | ||
|
|
||
| - (void) | ||
| httpsCallableStreamFromUrl:(NSString *)appName |
There was a problem hiding this comment.
similar to the java thought on largely-duplicated implementation - it looks like the largest portion of code which is actual code requiring implementation + maintenance on our part (that is, not boiler-plate, or method definitions, or generated code) is right here, and it's in 2 sizable chunks that appear nearly identical. Looks like an opportunity to reduce the future maintenance significantly by eliminating the repetition with a parameterized helper method that does the concrete implementation and is called by the two defined API methods we serve up to the upper layers
| declare module '@react-native-firebase/app/lib/internal/web/RNFBAppModule' { | ||
| interface RNFBAppModuleType { |
There was a problem hiding this comment.
it seems very strange to have this definition for an app thing here in this functions area 🤔
| } | ||
|
|
||
| // Map to store active streaming subscriptions by listenerId | ||
| // Subscription type from RxJS has an unsubscribe() method |
| this._useFunctionsEmulatorPort = -1; | ||
| this._id_functions_streaming_event = 0; | ||
|
|
||
| // @ts-ignore - emitter and eventNameForApp exist on FirebaseModule |
There was a problem hiding this comment.
ts-ignore --> but we just implemented types noooooo 😱 - we should fix the types so they're correct vs immediately corrupting / working around the type system
| } | ||
|
|
||
| const listenerId = this._id_functions_streaming_event++; | ||
| // @ts-ignore |
There was a problem hiding this comment.
same comment will apply to all ts-ignores - with a sparkly clean + fresh typescript implementation, and the ability to create types for things we need, ts-ignores should feel unacceptable at this point, with strong preference to fix types or create definitions as needed so we can use the type system instead of working around it
| }); | ||
| }; | ||
|
|
||
| callableFunction.stream = ( |
There was a problem hiding this comment.
looks duplicate when lined up with the above stream implementation ?
| const addLog = (message: string) => { | ||
| const timestamp = new Date().toLocaleTimeString(); | ||
| setLogs(prev => [`[${timestamp}] ${message}`, ...prev].slice(0, 50)); | ||
| console.log(`[StreamingTest] ${message}`); |
There was a problem hiding this comment.
console.log is something that's going to become more difficult to access in the future - it will go away from the metro console and only be visible in a separate / relatively heavy DevTools instance that you have to open independently
since the test is an active screen with a UI, could the logs been a scroll area directly visible on the screen instead? I think that'll be a much better DX going forward - we're only one dependency update (react-native-macos release...) from upgrading to a version of react-native in the test app where this will be our reality
| # Exclude Swift from source_files to avoid C++ dependency scanner issues | ||
| # Swift files must be added via Podfile post_install hook (see README or Podfile example) | ||
| # This is required because CocoaPods doesn't support automatic hooks from podspecs |
There was a problem hiding this comment.
Is there some explanation of what's going on here - that is, what 'C++ dependency scanner issues' exactly are being avoided, and is there some upstream issue logged?
This looks like an awful (tempted to say unacceptable really) developer experience to require library consumers to do a Podfile modification just to use this functions module after this. It implies more manual integration steps to document and answer issues for as well as some sort of Expo plugin that will need to be created and maintained
Especially if it's just because of using some Swift, when we are certainly not the first module to use Swift vs Objective-C 🤔
Additionally, there is some infra in react-native itself to run things that may help, for instance there is an ability to add things to build phases so scripts can run
If there truly is some insurmountable problem with the Obj-C++/Swift interop then perhaps the whole module could be ported to Swift - effort-wise that would probably be similar to the effort to correctly document this, answer the integration failure issues over time, and make the expo plugin
Basically, we need to do anything we can to avoid this
Description
streamimplementation forhttpsCallablefunctions #8210Related issues
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