(feat): add method to deallocate reactNativeFactory instance#301
Conversation
| #if !EXPO_SDK_GTE_55 | ||
| guard let factory = reactNativeFactory else { return } | ||
| factory.bridge?.invalidate() | ||
| #endif |
There was a problem hiding this comment.
This check is here because there is no bridge instance on Expo > 55?
There was a problem hiding this comment.
| private var factory: RCTReactNativeFactory { | ||
| if let existingFactory = reactNativeFactory { | ||
| return existingFactory | ||
| } | ||
|
|
||
| delegate.dependencyProvider = RCTAppDependencyProvider() | ||
| let createdFactory = ExpoReactNativeFactory(delegate: delegate) | ||
| reactNativeFactory = createdFactory | ||
| return createdFactory | ||
| } | ||
|
|
There was a problem hiding this comment.
We already create a factory instance in startReactNative and assign it as reactNativeFactory. So I believe we do not need to create another factory instance here. I know that it will only create a new instance if reactNativeFactory is nil BUT if we think about it, this instance will only be nil if startReactNative hasn't been called OR stopReactNative have been called already.
There was a problem hiding this comment.
@hurali97 some duplicated code removed in my second commit, please check it again
There was a problem hiding this comment.
@marcinszalski-callstack - Why do we need to create a separate factory variable instance? Why we can't re-use the reactNativeFactory?
I think this factory change is unnecessary and you use reactNativeFactory to call in stopReactNative. Similarly, I suggest keeping the factory initialization in startReactNative.
There was a problem hiding this comment.
in the original PR, factory was responsible for making sure a valid instance is always provided, creating a new instance if necessary.
in a next commit that logic to be simplified, i.e. to get rid of the reactNativeFactory property
| return reactNativeFactory?.rootViewFactory | ||
| }() | ||
|
|
||
| private var factory: RCTReactNativeFactory { |
There was a problem hiding this comment.
same here - please, @hurali97 , check my second commit
|
|
||
| guard let factory = reactNativeFactory else { return } | ||
|
|
||
| factory.bridge?.invalidate() |
There was a problem hiding this comment.
I believe we can skip this invocation as we do not support old arch anymore? And if I recall correctly on newer versions of RN, this factory.bridge should always be nil?
There was a problem hiding this comment.
I think we still do - we have a separate sourceset on Android.
Not all APIs support it (e.g. postMessage needs the New Arch).
I'd leave it as-is, unless you object?
There was a problem hiding this comment.
I recall we did a few PRs to remove the old arch support, when react-native decided to drop it. For instance, here - we previously provided a way for old arch but that was removed.
As part of this - we dropped support for old arch on Android - #148
I see that we still have a few places with old arch, we can plan removing those now.
There was a problem hiding this comment.
Alright, I also recall this. Let's have this change dropped then + we'll follow up with decoupling of OA remainders later
3 participants
Summary
Address the Add method to deallocate reactNativeFactory instance #134 issue
Test plan
See the Test plan from the #134 issue