(feat): add method to deallocate reactNativeFactory instance #301
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@callstack/react-native-brownfield': minor | ||
| --- | ||
|
|
||
| feat: add method to deallocate reactNativeFactory instance |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,7 @@ final class ReactNativeHostRuntime { | |
| delegate.bundlePath = bundlePath | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Bundle instance to lookup the JavaScript bundle. | ||
| * Default value: Bundle.main | ||
|
|
@@ -68,6 +69,7 @@ final class ReactNativeHostRuntime { | |
| delegate.bundle = bundle | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Dynamic bundle URL provider called on every bundle load. | ||
| * When set, this overrides the default bundleURL() behavior in the delegate. | ||
|
|
@@ -79,17 +81,23 @@ final class ReactNativeHostRuntime { | |
| delegate.bundleURLOverride = bundleURLOverride | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * React Native factory instance created when starting React Native. | ||
| * Default value: nil | ||
| */ | ||
| private var reactNativeFactory: RCTReactNativeFactory? = nil | ||
|
|
||
| private var factory: RCTReactNativeFactory { | ||
|
Contributor
Author
There was a problem hiding this comment. same here - please, @hurali97 , check my second commit |
||
| if let existingFactory = reactNativeFactory { | ||
| return existingFactory | ||
| } | ||
|
|
||
| delegate.dependencyProvider = RCTAppDependencyProvider() | ||
| let createdFactory = RCTReactNativeFactory(delegate: delegate) | ||
| reactNativeFactory = createdFactory | ||
| return createdFactory | ||
| } | ||
|
|
||
| /** | ||
| * Starts React Native with default parameters. | ||
|
|
@@ -98,12 +106,28 @@ final class ReactNativeHostRuntime { | |
| startReactNative(onBundleLoaded: nil) | ||
| } | ||
|
|
||
| /** | ||
| * Stops React Native and releases the underlying factory instance. | ||
| */ | ||
| public func stopReactNative() { | ||
| if !Thread.isMainThread { | ||
| DispatchQueue.main.async { [weak self] in self?.stopReactNative() } | ||
| return | ||
| } | ||
|
|
||
| guard let factory = reactNativeFactory else { return } | ||
|
|
||
| factory.bridge?.invalidate() | ||
|
Member
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
Collaborator
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. I'd leave it as-is, unless you object?
Member
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.
Collaborator
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 |
||
|
|
||
| reactNativeFactory = nil | ||
| } | ||
|
Comment on lines
+101
to
+108
Member
There was a problem hiding this comment. Did you accidentally remove the call to
Contributor
Author
There was a problem hiding this comment. yes, I guess that was a consequence of this topic: #301 (comment) should it be restored then?
Member
There was a problem hiding this comment. Ah no, they should not be restored. All good. One thing, can you also remove this from
Contributor
Author
There was a problem hiding this comment. ok, will be done in a next commit |
||
|
|
||
| public func view( | ||
| moduleName: String, | ||
| initialProps: [AnyHashable: Any]?, | ||
| launchOptions: [AnyHashable: Any]? = nil | ||
| ) -> UIView? { | ||
| factory.rootViewFactory.view( | ||
| withModuleName: moduleName, | ||
| initialProperties: initialProps, | ||
| launchOptions: launchOptions | ||
|
|
||
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.
We already create a factory instance in
startReactNativeand assign it asreactNativeFactory. So I believe we do not need to create anotherfactoryinstance here. I know that it will only create a new instance ifreactNativeFactoryisnilBUT if we think about it, this instance will only be nil ifstartReactNativehasn't been called ORstopReactNativehave been called already.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.
@hurali97 some duplicated code removed in my second commit, please check it again
Uh oh!
There was an error while loading. Please reload this page.
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.
@marcinszalski-callstack - Why do we need to create a separate factory variable instance? Why we can't re-use the
reactNativeFactory?I think this
factorychange is unnecessary and you usereactNativeFactoryto call instopReactNative. Similarly, I suggest keeping the factory initialization instartReactNative.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.
in the original PR,
factorywas 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
reactNativeFactoryproperty