From dd748ef574b1d7c3993d87e91e7a9122bcb042bf Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 10 Jun 2019 15:36:29 -0700 Subject: [PATCH] Moved logic to only send updated filters across Bridge to the Store (and added tests) --- src/__tests__/setupTests.js | 8 +++-- src/__tests__/storeComponentFilters-test.js | 33 +++++++++++++++++++ src/devtools/store.js | 29 ++++++++++++++-- .../views/Settings/ComponentsSettings.js | 25 +++----------- 4 files changed, 70 insertions(+), 25 deletions(-) diff --git a/src/__tests__/setupTests.js b/src/__tests__/setupTests.js index 2cd71ae731..18127c4d47 100644 --- a/src/__tests__/setupTests.js +++ b/src/__tests__/setupTests.js @@ -11,7 +11,10 @@ env.beforeEach(() => { const Bridge = require('src/bridge').default; const Store = require('src/devtools/store').default; const { installHook } = require('src/hook'); - const { getDefaultComponentFilters } = require('src/utils'); + const { + getDefaultComponentFilters, + saveComponentFilters, + } = require('src/utils'); // Fake timers let us flush Bridge operations between setup and assertions. jest.useFakeTimers(); @@ -26,7 +29,8 @@ env.beforeEach(() => { originalConsoleError.apply(console, args); }; - // Avoid "Invalid component filters" warning. + // Initialize filters to a known good state. + saveComponentFilters(getDefaultComponentFilters()); global.__REACT_DEVTOOLS_COMPONENT_FILTERS__ = getDefaultComponentFilters(); installHook(global); diff --git a/src/__tests__/storeComponentFilters-test.js b/src/__tests__/storeComponentFilters-test.js index 16b028abcb..3b7cfa5301 100644 --- a/src/__tests__/storeComponentFilters-test.js +++ b/src/__tests__/storeComponentFilters-test.js @@ -1,5 +1,6 @@ // @flow +import type Bridge from 'src/bridge'; import type Store from 'src/devtools/store'; describe('Store component filters', () => { @@ -7,6 +8,7 @@ describe('Store component filters', () => { let ReactDOM; let TestUtils; let Types; + let bridge: Bridge; let store: Store; let utils; @@ -18,6 +20,7 @@ describe('Store component filters', () => { }; beforeEach(() => { + bridge = global.bridge; store = global.store; store.collapseNodesByDefault = false; store.componentFilters = []; @@ -187,4 +190,34 @@ describe('Store component filters', () => { expect(store).toMatchSnapshot('3: disable HOC filter'); }); + + it('should not send a bridge update if the set of enabled filters has not changed', () => { + act(() => (store.componentFilters = [utils.createHOCFilter(true)])); + + bridge.addListener('updateComponentFilters', componentFilters => { + throw Error('Unexpected component update'); + }); + + act( + () => + (store.componentFilters = [ + utils.createHOCFilter(false), + utils.createHOCFilter(true), + ]) + ); + act( + () => + (store.componentFilters = [ + utils.createHOCFilter(true), + utils.createLocationFilter('abc', false), + ]) + ); + act( + () => + (store.componentFilters = [ + utils.createHOCFilter(true), + utils.createElementTypeFilter(Types.ElementTypeHostComponent, false), + ]) + ); + }); }); diff --git a/src/devtools/store.js b/src/devtools/store.js index 5e2e14d983..76ef1544e8 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -14,6 +14,7 @@ import { getSavedComponentFilters, saveComponentFilters, separateDisplayNameAndHOCs, + shallowDiffers, utfDecodeString, } from '../utils'; import { localStorageGetItem, localStorageSetItem } from '../storage'; @@ -242,14 +243,38 @@ export default class Store extends EventEmitter<{| throw Error('Cannot modify filter preferences while profiling'); } + // Filter updates are expensive to apply (since they impact the entire tree). + // Let's determine if they've changed and avoid doing this work if they haven't. + const prevEnabledComponentFilters = this._componentFilters.filter( + filter => filter.isEnabled + ); + const nextEnabledComponentFilters = value.filter( + filter => filter.isEnabled + ); + let haveEnabledFiltersChanged = + prevEnabledComponentFilters.length !== nextEnabledComponentFilters.length; + if (!haveEnabledFiltersChanged) { + for (let i = 0; i < nextEnabledComponentFilters.length; i++) { + const prevFilter = prevEnabledComponentFilters[i]; + const nextFilter = nextEnabledComponentFilters[i]; + if (shallowDiffers(prevFilter, nextFilter)) { + haveEnabledFiltersChanged = true; + break; + } + } + } + this._componentFilters = value; // Update persisted filter preferences stored in localStorage. saveComponentFilters(value); // Notify the renderer that filter prefernces have changed. - // This is an expensive opreation; it unmounts and remounts the entire tree. - this._bridge.send('updateComponentFilters', value); + // This is an expensive opreation; it unmounts and remounts the entire tree, + // so only do it if the set of enabled component filters has changed. + if (haveEnabledFiltersChanged) { + this._bridge.send('updateComponentFilters', value); + } this.emit('componentFilters'); } diff --git a/src/devtools/views/Settings/ComponentsSettings.js b/src/devtools/views/Settings/ComponentsSettings.js index 78f55e18aa..575a5226bb 100644 --- a/src/devtools/views/Settings/ComponentsSettings.js +++ b/src/devtools/views/Settings/ComponentsSettings.js @@ -14,7 +14,6 @@ import Store from 'src/devtools/store'; import Button from '../Button'; import ButtonIcon from '../ButtonIcon'; import Toggle from '../Toggle'; -import { shallowDiffers } from 'src/utils'; import { ComponentFilterDisplayName, ComponentFilterElementType, @@ -221,7 +220,9 @@ export default function ComponentsSettings(_: {||}) { ); // Filter updates are expensive to apply (since they impact the entire tree). - // Only apply them on unmount, and only if they've actually changed. + // Only apply them on unmount. + // The Store will avoid doing any expensive work unless they've changed. + // We just want to batch the work in the event that they do change. const componentFiltersRef = useRef>(componentFilters); useEffect(() => { componentFiltersRef.current = componentFilters; @@ -229,25 +230,7 @@ export default function ComponentsSettings(_: {||}) { }, [componentFilters]); useEffect( () => () => { - const prevComponentFilters = store.componentFilters; - const nextComponentFilters = componentFiltersRef.current; - - let haveFiltersChanged = - prevComponentFilters.length !== nextComponentFilters.length; - if (!haveFiltersChanged) { - for (let i = 0; i < nextComponentFilters.length; i++) { - if ( - shallowDiffers(prevComponentFilters[i], nextComponentFilters[i]) - ) { - haveFiltersChanged = true; - break; - } - } - } - - if (haveFiltersChanged) { - store.componentFilters = [...nextComponentFilters]; - } + store.componentFilters = [...componentFiltersRef.current]; }, [store] );