Run JS monitors on bridge for iOS #526
Conversation
louiszawadzki
force-pushed
the
louiszawadzki/fix-react-native-queue-ios
branch
from
a9dba5e
to
77f42e4
Compare
| self.jsRefreshRateMonitor = jsRefreshRateMonitor | ||
| super.init() | ||
| } | ||
|
|
||
| // Used only for tests | ||
| internal override convenience init() { |
There was a problem hiding this comment.
Might be better to add to test file as an extension to the type or some kind of simple factory function.
IMO it's better to not pollute main impl with test stuff.
There was a problem hiding this comment.
Awesome, I love the idea of using an extension!
| public convenience init(bridge: RCTBridge) { | ||
| self.init(mainDispatchQueue: DispatchQueue.main, bridge: bridge, jsRefreshRateMonitor: JSRefreshRateMonitor.init()) |
There was a problem hiding this comment.
it probably makes sense to have single init with defaults.
I'm guessing RCTBridge implements DispatchQueueType so we can probably drop bridge keyword and end up with something like:
init(
mainDispatchQueue: DispatchQueueType = DispatchQueue.main,
jsRefreshRateMonitor: RefreshRateMonitor = JSRefreshRateMonitor(),
jsDispatchQueue: DispatchQueueType
) {
...
}
There was a problem hiding this comment.
as discussed, not the easiest task to take on right now (we need to expose the Swift interfaces to Obj C++), but to look into it for the future
| @@ -377,7 +382,7 @@ public class DdSdkImplementation: NSObject { | |||
| func startJSRefreshRateMonitoring(sdkConfiguration: DdSdkConfiguration) { | |||
| if let frameTimeCallback = buildFrameTimeCallback(sdkConfiguration: sdkConfiguration) { | |||
| // Falling back to mainDispatchQueue if bridge is nil is only useful for tests | |||
| self.jsRefreshRateMonitor.startMonitoring(jsQueue: bridge ?? mainDispatchQueue, frameTimeCallback: frameTimeCallback) | |||
| self.jsRefreshRateMonitor.startMonitoring(jsQueue: jsDispatchQueue ?? mainDispatchQueue, frameTimeCallback: frameTimeCallback) | |||
There was a problem hiding this comment.
I assume this is the actual fix, I think I might be missing some context here 🤔
Bridge was never set in previous implementation? Wasn't it crashing hence force unwrap? 🤔
| @@ -25,11 +32,11 @@ internal class DdSdkTests: XCTestCase { | |||
| var printedMessage = "" | |||
| consolePrint = { msg in printedMessage += msg } | |||
|
|
|||
| DdSdkImplementation(mainDispatchQueue: DispatchQueueMock(), jsRefreshRateMonitor: JSRefreshRateMonitor()).initialize(configuration: .mockAny(), resolve: mockResolve, reject: mockReject) | |||
| DdSdkImplementation(mainDispatchQueue: DispatchQueueMock(), bridge: DispatchQueueMock(), jsRefreshRateMonitor: JSRefreshRateMonitor()).initialize(configuration: .mockAny(), resolve: mockResolve, reject: mockReject) | |||
There was a problem hiding this comment.
might be a good idea to format break those lengthy lines
There was a problem hiding this comment.
I need to add a linter for swift files, I think this should be in a separate PR to avoid polluting this one
|
|
||
| DdSdkImplementation(mainDispatchQueue: DispatchQueueMock(), bridge: bridge, jsRefreshRateMonitor: mockJSRefreshRateMonitor).initialize(configuration: .mockAny(longTaskThresholdMs: 0.2), resolve: mockResolve, reject: mockReject) | ||
|
|
||
| XCTAssertTrue(bridge.isSameQueue(queue: mockJSRefreshRateMonitor.jsQueue!)) |
There was a problem hiding this comment.
Might be personal preference here, but in reality we're checking if:
jsRefreshRateMonitor.startMonitoring() was called with correct parameters.
Maybe worth injecting spy JSRefreshRateMonitor and check this instead.
There was a problem hiding this comment.
as discussed, not the easiest task to take on right now and it won't bring much value, but to look into it for the future
| @@ -17,24 +17,29 @@ func getDefaultAppVersion() -> String { | |||
|
|
|||
| @objc | |||
| public class DdSdkImplementation: NSObject { | |||
| @objc var bridge: RCTBridge! | |||
There was a problem hiding this comment.
This line was not working anymore with the RN new arch migration
| @@ -79,7 +81,7 @@ @implementation DdSdk | |||
| - (DdSdkImplementation*)ddSdkImplementation | |||
| { | |||
| if (_ddSdkImplementation == nil) { | |||
| _ddSdkImplementation = [[DdSdkImplementation alloc] init]; | |||
| _ddSdkImplementation = [[DdSdkImplementation alloc] initWithBridge:_bridge]; | |||
There was a problem hiding this comment.
This is the actual fix, we're getting the bridge here (line 17) as we can do it because this is the RN module.
Then we're passing the bridge to the DdSdkImplementation
louiszawadzki
force-pushed
the
louiszawadzki/fix-react-native-queue-ios
branch
from
77f42e4
to
cd7bfcd
Compare
What does this PR do?
This PR fixes a regression where JS thread monitors (reporting JS long tasks and JS FPS) were running on the main queue on iOS instead of the JS bridge.
Motivation
After the migration to the new React Native architecture, we were not running the iOS JS thread monitors on the bridge but on the main queue.
By default JS thread monitoring is not enabled so that explains that it was not reported by users. However, we now display the JS framerate on all views where it is reported, so it will probably change.
I noticed a lot of duplicated long tasks when testing the v2 migration and traced the origin of the bug to 1.8 version:
I want to get this change out before the v2 migration to bring it to as many people as possible.
Review checklist (to be filled by reviewers)