From 586d55d54f21e0fe6a0a0230a1b407693ba7d193 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Fri, 13 Dec 2019 03:07:06 -0800 Subject: [PATCH] LogBox - lazily initialize on iOS, use sync APIs Summary: Update LogBox on iOS to lazily initialize, using a synchronous RCTSurface, behind RCTSharedApplication checks. This results in faster of LogBox, without keeping around a long lived window in the background, and only used when LogBox is used. On Android, we still start the react app in the background but we create a dialog when it's shown and then destroy it when it's hidden. Once we have the sync APIs on android we can update it to use the same strategy. Changelog: [Internal] Reviewed By: fkgozali Differential Revision: D18925538 fbshipit-source-id: 1a72c39aa0fc26c8ba657d36c7fa7bc0ae777eb9 --- Libraries/LogBox/Data/LogBoxData.js | 58 +++++---- Libraries/LogBox/LogBoxInspectorContainer.js | 36 ++---- Libraries/LogBox/UI/LogBoxInspector.js | 2 +- Libraries/LogBox/UI/LogBoxInspectorFooter.js | 6 +- .../LogBoxInspectorFooter-test.js.snap | 6 +- .../LogBoxNotificationContainer-test.js.snap | 114 +++++++++--------- React/CoreModules/RCTLogBox.mm | 95 +++++++-------- 7 files changed, 151 insertions(+), 166 deletions(-) diff --git a/Libraries/LogBox/Data/LogBoxData.js b/Libraries/LogBox/Data/LogBoxData.js index 2ca6f56a640248..1ee81e5a635104 100644 --- a/Libraries/LogBox/Data/LogBoxData.js +++ b/Libraries/LogBox/Data/LogBoxData.js @@ -22,7 +22,7 @@ import type { } from './parseLogBoxLog'; import parseErrorStack from '../../Core/Devtools/parseErrorStack'; import type {ExtendedError} from '../../Core/Devtools/parseErrorStack'; - +import NativeLogBox from '../../NativeModules/specs/NativeLogBox'; export type LogBoxLogs = Set; export type LogData = $ReadOnly<{| level: LogLevel, @@ -86,20 +86,6 @@ const LOGBOX_ERROR_MESSAGE = 'An error was thrown when attempting to render log messages via LogBox.'; function getNextState() { - const logArray = Array.from(logs); - let index = logArray.length - 1; - while (index >= 0) { - // The latest syntax error is selected and displayed before all other logs. - if (logArray[index].level === 'syntax') { - return { - logs, - isDisabled: _isDisabled, - selectedLogIndex: index, - }; - } - index -= 1; - } - return { logs, isDisabled: _isDisabled, @@ -175,9 +161,10 @@ function appendNewLog(newLog) { let addPendingLog = () => { logs.add(newLog); if (_selectedIndex <= 0) { - _selectedIndex = logs.size - 1; + setSelectedLog(logs.size - 1); + } else { + handleUpdate(); } - handleUpdate(); addPendingLog = null; }; @@ -196,6 +183,9 @@ function appendNewLog(newLog) { handleUpdate(); } }); + } else if (newLog.level === 'syntax') { + logs.add(newLog); + setSelectedLog(logs.size - 1); } else { logs.add(newLog); handleUpdate(); @@ -259,21 +249,42 @@ export function symbolicateLogLazy(log: LogBoxLog) { export function clear(): void { if (logs.size > 0) { logs = new Set(); - _selectedIndex = -1; - handleUpdate(); + setSelectedLog(-1); } } -export function setSelectedLog(index: number): void { - _selectedIndex = index; +export function setSelectedLog(proposedNewIndex: number): void { + const oldIndex = _selectedIndex; + let newIndex = proposedNewIndex; + + const logArray = Array.from(logs); + let index = logArray.length - 1; + while (index >= 0) { + // The latest syntax error is selected and displayed before all other logs. + if (logArray[index].level === 'syntax') { + newIndex = index; + break; + } + index -= 1; + } + _selectedIndex = newIndex; handleUpdate(); + if (NativeLogBox) { + setTimeout(() => { + if (oldIndex < 0 && newIndex >= 0) { + NativeLogBox.show(); + } else if (oldIndex >= 0 && newIndex < 0) { + NativeLogBox.hide(); + } + }, 0); + } } export function clearWarnings(): void { const newLogs = Array.from(logs).filter(log => log.level !== 'warn'); if (newLogs.length !== logs.size) { logs = new Set(newLogs); - _selectedIndex = -1; + setSelectedLog(-1); handleUpdate(); } } @@ -284,8 +295,7 @@ export function clearErrors(): void { ); if (newLogs.length !== logs.size) { logs = new Set(newLogs); - _selectedIndex = -1; - handleUpdate(); + setSelectedLog(-1); } } diff --git a/Libraries/LogBox/LogBoxInspectorContainer.js b/Libraries/LogBox/LogBoxInspectorContainer.js index c8bfccff28826a..35f19628810e89 100644 --- a/Libraries/LogBox/LogBoxInspectorContainer.js +++ b/Libraries/LogBox/LogBoxInspectorContainer.js @@ -15,7 +15,6 @@ import {View, StyleSheet} from 'react-native'; import * as LogBoxData from './Data/LogBoxData'; import LogBoxInspector from './UI/LogBoxInspector'; import type LogBoxLog from './Data/LogBoxLog'; -import NativeLogBox from '../NativeModules/specs/NativeLogBox'; type Props = $ReadOnly<{| logs: $ReadOnlyArray, @@ -23,35 +22,18 @@ type Props = $ReadOnly<{| isDisabled?: ?boolean, |}>; -function NativeLogBoxVisibility(props) { - React.useLayoutEffect(() => { - if (NativeLogBox) { - if (props.visible) { - // Schedule this to try and prevent flashing the old state. - setTimeout(() => NativeLogBox.show(), 10); - } else { - NativeLogBox.hide(); - } - } - }, [props.visible]); - - return props.children; -} - export class _LogBoxInspectorContainer extends React.Component { render(): React.Node { return ( - = 0}> - - - - + + + ); } diff --git a/Libraries/LogBox/UI/LogBoxInspector.js b/Libraries/LogBox/UI/LogBoxInspector.js index bf11270c3bbbf6..62900e5b47ca2d 100644 --- a/Libraries/LogBox/UI/LogBoxInspector.js +++ b/Libraries/LogBox/UI/LogBoxInspector.js @@ -36,8 +36,8 @@ type Props = $ReadOnly<{| function LogBoxInspector(props: Props): React.Node { const {logs, selectedIndex} = props; + let log = logs[selectedIndex]; - const log = logs[selectedIndex]; React.useEffect(() => { if (log) { LogBoxData.symbolicateLogNow(log); diff --git a/Libraries/LogBox/UI/LogBoxInspectorFooter.js b/Libraries/LogBox/UI/LogBoxInspectorFooter.js index 82daf58d707eca..f0013f5fa8934d 100644 --- a/Libraries/LogBox/UI/LogBoxInspectorFooter.js +++ b/Libraries/LogBox/UI/LogBoxInspectorFooter.js @@ -101,9 +101,11 @@ const styles = StyleSheet.create({ syntaxErrorText: { textAlign: 'center', width: '100%', + height: 48, fontSize: 14, - paddingTop: 15, - paddingBottom: 15, + lineHeight: 20, + paddingTop: 20, + paddingBottom: 50, fontStyle: 'italic', color: LogBoxStyle.getTextColor(0.6), }, diff --git a/Libraries/LogBox/UI/__tests__/__snapshots__/LogBoxInspectorFooter-test.js.snap b/Libraries/LogBox/UI/__tests__/__snapshots__/LogBoxInspectorFooter-test.js.snap index 48e29aa5ae76b3..958f456d53f568 100644 --- a/Libraries/LogBox/UI/__tests__/__snapshots__/LogBoxInspectorFooter-test.js.snap +++ b/Libraries/LogBox/UI/__tests__/__snapshots__/LogBoxInspectorFooter-test.js.snap @@ -29,8 +29,10 @@ exports[`LogBoxInspectorFooter should render no buttons and a message for syntax "color": "rgba(255, 255, 255, 0.6)", "fontSize": 14, "fontStyle": "italic", - "paddingBottom": 15, - "paddingTop": 15, + "height": 48, + "lineHeight": 20, + "paddingBottom": 50, + "paddingTop": 20, "textAlign": "center", "width": "100%", } diff --git a/Libraries/LogBox/__tests__/__snapshots__/LogBoxNotificationContainer-test.js.snap b/Libraries/LogBox/__tests__/__snapshots__/LogBoxNotificationContainer-test.js.snap index e98a8765fe1277..2c2253ad83b26b 100644 --- a/Libraries/LogBox/__tests__/__snapshots__/LogBoxNotificationContainer-test.js.snap +++ b/Libraries/LogBox/__tests__/__snapshots__/LogBoxNotificationContainer-test.js.snap @@ -1,66 +1,62 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`LogBoxNotificationContainer should render inspector with logs, even when disabled 1`] = ` - - - + - - + "stack": Array [], + "symbolicated": Object { + "error": null, + "stack": null, + "status": "NONE", + }, + }, + ] + } + onChangeSelectedIndex={[Function]} + onDismiss={[Function]} + onMinimize={[Function]} + selectedIndex={-1} + /> + `; diff --git a/React/CoreModules/RCTLogBox.mm b/React/CoreModules/RCTLogBox.mm index 0041ed283520aa..0b09c4163a6910 100644 --- a/React/CoreModules/RCTLogBox.mm +++ b/React/CoreModules/RCTLogBox.mm @@ -17,6 +17,8 @@ #import #import #import +#import +#import #import #import @@ -25,61 +27,52 @@ #if RCT_DEV_MENU -@class RCTLogBoxWindow; - -@protocol RCTLogBoxWindowActionDelegate - -- (void)logBoxWindow:(RCTLogBoxWindow *)logBoxWindow openStackFrameInEditor:(RCTJSStackFrame *)stackFrame; -- (void)reloadFromlogBoxWindow:(RCTLogBoxWindow *)logBoxWindow; -- (void)loadExtraDataViewController; +@class RCTLogBoxView; +@interface RCTLogBoxView : UIView @end -@interface RCTLogBoxWindow : UIWindow -@property (nonatomic, weak) id actionDelegate; -@end - -@implementation RCTLogBoxWindow +@implementation RCTLogBoxView { - UITableView *_stackTraceTableView; - NSString *_lastErrorMessage; - NSArray *_lastStackTrace; - int _lastErrorCookie; + UIViewController *_rootViewController; + RCTSurface *_surface; } - (instancetype)initWithFrame:(CGRect)frame bridge:(RCTBridge *)bridge { - _lastErrorCookie = -1; if ((self = [super initWithFrame:frame])) { -#if TARGET_OS_TV - self.windowLevel = UIWindowLevelAlert + 1000; -#else - self.windowLevel = UIWindowLevelStatusBar - 1; -#endif self.backgroundColor = [UIColor clearColor]; - self.hidden = YES; - RCTRootView *rootView = [[RCTRootView alloc] initWithBridge:bridge moduleName:@"LogBox" initialProperties:nil]; + _surface = [[RCTSurface alloc] initWithBridge:bridge moduleName:@"LogBox" initialProperties:@{}]; + + [_surface start]; + [_surface setSize:frame.size]; - UIViewController *rootViewController = [UIViewController new]; - rootViewController.view = rootView; - self.rootViewController = rootViewController; + if (![_surface synchronouslyWaitForStage:RCTSurfaceStageSurfaceDidInitialMounting timeout:.5]) { + RCTLogInfo(@"Failed to mount LogBox within 500ms"); + } + + _rootViewController = [UIViewController new]; + _rootViewController.view = (UIView *)_surface.view; + _rootViewController.view.backgroundColor = [UIColor clearColor]; + _rootViewController.modalPresentationStyle = UIModalPresentationFullScreen; } return self; } - -- (void)show +- (void)dealloc { - [self becomeFirstResponder]; - [self makeKeyAndVisible]; + // Dismiss by deallocating the window. + // This will also handle JS reload, otherwise the LogBox view would be stuck on top. + [_rootViewController.view resignFirstResponder]; + [_rootViewController dismissViewControllerAnimated:NO completion:NULL]; } -- (void)dismiss +- (void)show { - self.hidden = YES; - [self resignFirstResponder]; - [RCTSharedApplication().delegate.window makeKeyWindow]; + [RCTSharedApplication().delegate.window.rootViewController presentViewController:_rootViewController animated:NO completion:^{ + [self->_rootViewController.view becomeFirstResponder]; + }]; } @end @@ -89,7 +82,7 @@ @interface RCTLogBox () @implementation RCTLogBox { - RCTLogBoxWindow *_window; + RCTLogBoxView *_view; } @synthesize bridge = _bridge; @@ -101,34 +94,34 @@ + (BOOL)requiresMainQueueSetup return YES; } -- (void)setBridge:(RCTBridge *)bridge +RCT_EXPORT_METHOD(show) { - _bridge = bridge; - if (RCTRedBoxGetEnabled()) { dispatch_async(dispatch_get_main_queue(), ^{ - self->_window = [[RCTLogBoxWindow alloc] initWithFrame:[UIScreen mainScreen].bounds bridge: self->_bridge]; + if (!self->_view) { + self->_view = [[RCTLogBoxView alloc] initWithFrame:[UIScreen mainScreen].bounds bridge: self->_bridge]; + } + [self->_view show]; }); } } -RCT_EXPORT_METHOD(show) -{ - dispatch_async(dispatch_get_main_queue(), ^{ - [self->_window show]; - }); -} - RCT_EXPORT_METHOD(hide) { - dispatch_async(dispatch_get_main_queue(), ^{ - [self->_window dismiss]; - }); + if (RCTRedBoxGetEnabled()) { + dispatch_async(dispatch_get_main_queue(), ^{ + self->_view = nil; + }); + } } - (std::shared_ptr)getTurboModuleWithJsInvoker:(std::shared_ptr)jsInvoker { - return std::make_shared(self, jsInvoker); + if (RCTRedBoxGetEnabled()) { + return std::make_shared(self, jsInvoker); + } + + return nullptr; } @end