From 133788380e4af45d59a1fa15d05d40c0f2d59354 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20B=C3=BCchele?= Date: Fri, 30 Nov 2018 09:37:39 -0800 Subject: [PATCH] prevent unnecessary rerenders Summary: Updates of the redux store caused rerendering of UI components, even without the acutal UI changing. This diff ensures the UI is only updated, when The PluginContainer received all plugin states and selected the pluginState of the active plugin to pass it down to the plugin. However, this caused a rerender everytime a pluginState changed (even if the plugin was in the background). This diff moves the selection of the active plugin to the `connect`-function and only passes the state of the active plugin into the container. This makes sure the plugin rerenders only if it's own state changes. The main sidebar displays the number of notifications. Therefore, it was passed the array of notifications. However, this array is regenerated, everytime a new notification **might** be triggered. Now, the number of notifications is calculated in the `connect`-method and only the number itself is passed into the component. This makes sure the sidebar is only rerendered, when the actual number of notifications changes. Reviewed By: passy Differential Revision: D13276096 fbshipit-source-id: bf1e6c4a186f7a1cf7f7427cd3523b5b71eb003a --- src/PluginContainer.js | 131 ++++++++++++++++---------------------- src/chrome/MainSidebar.js | 27 ++++---- 2 files changed, 68 insertions(+), 90 deletions(-) diff --git a/src/PluginContainer.js b/src/PluginContainer.js index 564d468c0..9d7d9632e 100644 --- a/src/PluginContainer.js +++ b/src/PluginContainer.js @@ -12,7 +12,7 @@ import type {Props as PluginProps} from './plugin'; import Client from './Client.js'; import { ErrorBoundary, - Component, + PureComponent, FlexColumn, FlexRow, colors, @@ -40,71 +40,24 @@ const SidebarContainer = styled(FlexRow)({ type Props = { logger: LogManager, - selectedDevice: BaseDevice, - selectedPlugin: ?string, - selectedApp: ?string, - pluginStates: { - [pluginKey: string]: Object, - }, - clients: Array, - setPluginState: (payload: { - pluginKey: string, - state: Object, - }) => void, + pluginState: Object, + activePlugin: ?Class | FlipperDevicePlugin<>>, + target: Client | BaseDevice | null, + pluginKey: string, deepLinkPayload: ?string, + selectPlugin: (payload: {| selectedPlugin: ?string, selectedApp?: ?string, deepLinkPayload: ?string, |}) => mixed, - devicePlugins: Map>>, - clientPlugins: Map>>, + setPluginState: (payload: { + pluginKey: string, + state: Object, + }) => void, }; -type State = { - activePlugin: ?Class | FlipperDevicePlugin<>>, - target: Client | BaseDevice | null, - pluginKey: string, -}; - -class PluginContainer extends Component { - static getDerivedStateFromProps(props: Props): State { - const {selectedPlugin} = props; - let pluginKey = 'unknown'; - let target = null; - let activePlugin: ?Class | FlipperDevicePlugin<>> = null; - - if (selectedPlugin) { - if (selectedPlugin === NotificationsHub.id) { - activePlugin = NotificationsHub; - } else if (props.selectedPlugin) { - activePlugin = props.devicePlugins.get(props.selectedPlugin); - } - target = props.selectedDevice; - if (activePlugin) { - pluginKey = `${props.selectedDevice.serial}#${activePlugin.id}`; - } else { - target = props.clients.find( - (client: Client) => client.id === props.selectedApp, - ); - activePlugin = props.clientPlugins.get(selectedPlugin); - if (!activePlugin || !target) { - throw new Error( - `Plugin "${props.selectedPlugin || ''}" could not be found.`, - ); - } - pluginKey = `${target.id}#${activePlugin.id}`; - } - } - - return { - activePlugin, - target, - pluginKey, - }; - } - - state: State = this.constructor.getDerivedStateFromProps(this.props); +class PluginContainer extends PureComponent { plugin: ?FlipperPlugin<> | FlipperDevicePlugin<>; refChanged = (ref: ?FlipperPlugin<> | FlipperDevicePlugin<>) => { @@ -112,8 +65,7 @@ class PluginContainer extends Component { this.plugin._teardown(); this.plugin = null; } - const {target} = this.state; - if (ref && target) { + if (ref && this.props.target) { activateMenuItems(ref); ref._init(); this.props.logger.trackTimeSince(`activePlugin-${ref.constructor.id}`); @@ -122,8 +74,13 @@ class PluginContainer extends Component { }; render() { - const {pluginStates, setPluginState} = this.props; - const {activePlugin, pluginKey, target} = this.state; + const { + pluginState, + setPluginState, + activePlugin, + pluginKey, + target, + } = this.props; if (!activePlugin || !target) { return null; @@ -134,14 +91,14 @@ class PluginContainer extends Component { persistedState: activePlugin.defaultPersistedState ? { ...activePlugin.defaultPersistedState, - ...pluginStates[pluginKey], + ...pluginState, } - : pluginStates[pluginKey], + : pluginState, setPersistedState: state => setPluginState({pluginKey, state}), target, deepLinkPayload: this.props.deepLinkPayload, selectPlugin: (pluginID: string, deepLinkPayload: ?string) => { - const {target} = this.state; + const {target} = this.props; // check if plugin will be available if ( target instanceof Client && @@ -192,16 +149,40 @@ export default connect( }, pluginStates, plugins: {devicePlugins, clientPlugins}, - }) => ({ - selectedPlugin, - selectedDevice, - pluginStates, - selectedApp, - clients, - deepLinkPayload, - devicePlugins, - clientPlugins, - }), + }) => { + let pluginKey = 'unknown'; + let target = null; + let activePlugin: ?Class | FlipperDevicePlugin<>> = null; + + if (selectedPlugin) { + if (selectedPlugin === NotificationsHub.id) { + activePlugin = NotificationsHub; + } else if (selectedPlugin) { + activePlugin = devicePlugins.get(selectedPlugin); + } + target = selectedDevice; + if (activePlugin) { + pluginKey = `${selectedDevice.serial}#${activePlugin.id}`; + } else { + target = clients.find((client: Client) => client.id === selectedApp); + activePlugin = clientPlugins.get(selectedPlugin); + if (!activePlugin || !target) { + throw new Error( + `Plugin "${selectedPlugin || ''}" could not be found.`, + ); + } + pluginKey = `${target.id}#${activePlugin.id}`; + } + } + + return { + pluginState: pluginStates[pluginKey], + activePlugin, + target, + deepLinkPayload, + pluginKey, + }; + }, { setPluginState, selectPlugin, diff --git a/src/chrome/MainSidebar.js b/src/chrome/MainSidebar.js index c39bfdfc2..3f64821f5 100644 --- a/src/chrome/MainSidebar.js +++ b/src/chrome/MainSidebar.js @@ -12,6 +12,7 @@ import type {UninitializedClient} from '../UninitializedClient.js'; import type {PluginNotification} from '../reducers/notifications'; import { + PureComponent, Component, Sidebar, FlexBox, @@ -177,13 +178,12 @@ type MainSidebarProps = {| client: UninitializedClient, deviceId?: string, }>, - activeNotifications: Array, - blacklistedPlugins: Array, + numNotifications: number, devicePlugins: Map>>, clientPlugins: Map>>, |}; -class MainSidebar extends Component { +class MainSidebar extends PureComponent { render() { const { selectedDevice, @@ -191,7 +191,7 @@ class MainSidebar extends Component { selectedApp, selectPlugin, windowIsFocused, - activeNotifications, + numNotifications, } = this.props; let {clients, uninitializedClients} = this.props; @@ -202,11 +202,6 @@ class MainSidebar extends Component { ) .sort((a, b) => (a.query.app || '').localeCompare(b.query.app)); - let blacklistedPlugins = new Set(this.props.blacklistedPlugins); - const notifications = activeNotifications.filter( - (n: PluginNotification) => !blacklistedPlugins.has(n.pluginId), - ); - return ( { }> 0 ? NotificationsHub.icon : 'bell-null' - } + name={numNotifications > 0 ? NotificationsHub.icon : 'bell-null'} isActive={selectedPlugin === NotificationsHub.id} /> {NotificationsHub.title} @@ -322,8 +315,12 @@ export default connect( notifications: {activeNotifications, blacklistedPlugins}, plugins: {devicePlugins, clientPlugins}, }) => ({ - blacklistedPlugins, - activeNotifications, + numNotifications: (() => { + const blacklist = new Set(blacklistedPlugins); + return activeNotifications.filter( + (n: PluginNotification) => !blacklist.has(n.pluginId), + ).length; + })(), windowIsFocused, selectedDevice, selectedPlugin,