From f3e06f46f3d9fe5c2be109a903d90ca0fb25433b Mon Sep 17 00:00:00 2001 From: Razvan Caliman Date: Thu, 29 Oct 2020 14:35:55 +0000 Subject: [PATCH] Bug 1623906 - [devtools] Dispatch thunks from Flexbox panel React components to highlight nodes r=gl,jdescottes,nchevobbe ### Problem There is a lot of [prop drilling](https://kentcdodds.com/blog/prop-drilling) in React components to pass down a callback that is invoked at the bottom of a long chain of components. [onShowBoxModelHighlighterForNode()](https://searchfox.org/mozilla-central/search?path=&q=onShowBoxModelHighlighterForNode) is one such example. It is created at the [Inspector client level](https://searchfox.org/mozilla-central/rev/25d491b7924493b5d4bedc07841a1cd92f271100/devtools/client/inspector/inspector.js#1885-1908) then passed to React components for panels, then drilled down to the consumer component which invokes it. There is also some [needless duplication](https://searchfox.org/mozilla-central/source/devtools/client/inspector/layout/layout.js#85-86) with [onShowBoxModelHighlighter](https://searchfox.org/mozilla-central/search?q=onShowBoxModelHighlighter%5CW&path=&case=false®exp=true). ### Solution With this patch, we leverage thunks in Redux. In Redux, you can `dispatch()`: - actions -> an object with an action type string which is matched by reducers - thunks -> a function which can be async and can itself dispatch actions or other thunks Thunks are supported by middleware already set up in the DevTools Redux [createStore()](https://searchfox.org/mozilla-central/source/devtools/client/shared/redux/create-store.js#54,104) helper. During store setup, we pass `thunkOptions` to the middleware, an object with arguments which will be available to all thunks when invoked. This is where we pass in the inspector client as a thunk option so we can invoke the highlighter. This is the replacement for prop drilling the `onShowBoxModelHighlighterForNode()` method. The same way they dispatch actions, components can now dispatch the thunk to show/hide the highlighter when they need it without direct knowledge of the `inspector`, thus satisfying the original intent of passing down the `onShowBoxModelHighlighterForNode()` callback while cleaning up the code. ### Prior art Thunks are not something new to DevTools. They are extensively used by the WebConsole, for example: - passing [thunk options to the store](https://searchfox.org/mozilla-central/rev/25d491b7924493b5d4bedc07841a1cd92f271100/devtools/client/webconsole/webconsole-wrapper.js#95-104) - making use of thunk options, [webConsoleUI and hud](https://searchfox.org/mozilla-central/source/devtools/client/webconsole/actions/autocomplete.js#14-91) in a thunk to handle autocomplete ###Useful context: DevTools helpers - [thunkWithOptions](https://searchfox.org/mozilla-central/source/devtools/client/shared/redux/middleware/thunk-with-options.js) - [shared Redux createStore helper with thunks middleware](https://searchfox.org/mozilla-central/source/devtools/client/shared/redux/create-store.js#54,104) Redux docs - [Async Actions in Redux](https://redux.js.org/advanced/async-actions) - [dispatch() as default prop with redux-connect](https://react-redux.js.org/using-react-redux/connect-mapdispatch#default-dispatch-as-a-prop) Differential Revision: https://phabricator.services.mozilla.com/D79556 --- .../boxmodel/actions/box-model-highlighter.js | 48 +++++++++++++++++++ .../inspector/boxmodel/actions/moz.build | 1 + .../flexbox/components/FlexContainer.js | 28 ++++------- .../inspector/flexbox/components/FlexItem.js | 17 ++++--- .../flexbox/components/FlexItemList.js | 14 ++---- .../inspector/flexbox/components/Flexbox.js | 23 ++++----- .../inspector/flexbox/components/Header.js | 21 +++----- devtools/client/inspector/inspector.js | 2 +- .../inspector/layout/components/LayoutApp.js | 1 + devtools/client/inspector/store.js | 6 ++- 10 files changed, 94 insertions(+), 67 deletions(-) create mode 100644 devtools/client/inspector/boxmodel/actions/box-model-highlighter.js diff --git a/devtools/client/inspector/boxmodel/actions/box-model-highlighter.js b/devtools/client/inspector/boxmodel/actions/box-model-highlighter.js new file mode 100644 index 000000000000..340c03e5ed98 --- /dev/null +++ b/devtools/client/inspector/boxmodel/actions/box-model-highlighter.js @@ -0,0 +1,48 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +"use strict"; + +/** + * This module exports thunks. + * Thunks are functions that can be dispatched to the Inspector Redux store. + * + * These functions receive one object with options that contains: + * - dispatch() => function to dispatch Redux actions to the store + * - getState() => function to get the current state of the entire Inspector Redux store + * - inspector => object instance of Inspector client + * + * They provide a shortcut for React components to invoke the box model highlighter + * without having to know where the highlighter exists. + */ + +module.exports = { + /** + * Show the box model highlighter for the given node front. + * + * @param {NodeFront} nodeFront + * Node that should be highlighted. + */ + highlightNode(nodeFront) { + return async thunkOptions => { + const { inspector } = thunkOptions; + await inspector.highlighters.showHighlighterTypeForNode( + inspector.highlighters.TYPES.BOXMODEL, + nodeFront + ); + }; + }, + + /** + * Hide the box model highlighter for any highlighted node. + */ + unhighlightNode() { + return async thunkOptions => { + const { inspector } = thunkOptions; + await inspector.highlighters.hideHighlighterType( + inspector.highlighters.TYPES.BOXMODEL + ); + }; + }, +}; diff --git a/devtools/client/inspector/boxmodel/actions/moz.build b/devtools/client/inspector/boxmodel/actions/moz.build index 0bc9633762a6..7ee681b35c25 100644 --- a/devtools/client/inspector/boxmodel/actions/moz.build +++ b/devtools/client/inspector/boxmodel/actions/moz.build @@ -5,6 +5,7 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. DevToolsModules( + "box-model-highlighter.js", "box-model.js", "index.js", ) diff --git a/devtools/client/inspector/flexbox/components/FlexContainer.js b/devtools/client/inspector/flexbox/components/FlexContainer.js index 8bb65d442dd5..f2148e44ae51 100644 --- a/devtools/client/inspector/flexbox/components/FlexContainer.js +++ b/devtools/client/inspector/flexbox/components/FlexContainer.js @@ -12,7 +12,6 @@ const { } = require("devtools/client/shared/vendor/react"); const dom = require("devtools/client/shared/vendor/react-dom-factories"); const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); -const { connect } = require("devtools/client/shared/vendor/react-redux"); loader.lazyRequireGetter( this, @@ -22,15 +21,19 @@ loader.lazyRequireGetter( const Types = require("devtools/client/inspector/flexbox/types"); +const { + highlightNode, + unhighlightNode, +} = require("devtools/client/inspector/boxmodel/actions/box-model-highlighter"); + class FlexContainer extends PureComponent { static get propTypes() { return { + dispatch: PropTypes.func.isRequired, color: PropTypes.string.isRequired, flexContainer: PropTypes.shape(Types.flexContainer).isRequired, getSwatchColorPickerTooltip: PropTypes.func.isRequired, - onHideBoxModelHighlighter: PropTypes.func.isRequired, onSetFlexboxOverlayColor: PropTypes.func.isRequired, - onShowBoxModelHighlighterForNode: PropTypes.func.isRequired, }; } @@ -69,12 +72,7 @@ class FlexContainer extends PureComponent { } render() { - const { - color, - flexContainer, - onHideBoxModelHighlighter, - onShowBoxModelHighlighterForNode, - } = this.props; + const { color, flexContainer, dispatch } = this.props; const { nodeFront, properties } = flexContainer; return createElement( @@ -85,8 +83,8 @@ class FlexContainer extends PureComponent { className: "flex-header-container-label", }, getNodeRep(nodeFront, { - onDOMNodeMouseOut: () => onHideBoxModelHighlighter(), - onDOMNodeMouseOver: () => onShowBoxModelHighlighterForNode(nodeFront), + onDOMNodeMouseOut: () => dispatch(unhighlightNode()), + onDOMNodeMouseOver: () => dispatch(highlightNode(nodeFront)), }), dom.div({ className: "layout-color-swatch", @@ -121,10 +119,4 @@ class FlexContainer extends PureComponent { } } -const mapStateToProps = state => { - return { - color: state.flexbox.color, - }; -}; - -module.exports = connect(mapStateToProps)(FlexContainer); +module.exports = FlexContainer; diff --git a/devtools/client/inspector/flexbox/components/FlexItem.js b/devtools/client/inspector/flexbox/components/FlexItem.js index a7ef144371ba..b618e142964d 100644 --- a/devtools/client/inspector/flexbox/components/FlexItem.js +++ b/devtools/client/inspector/flexbox/components/FlexItem.js @@ -16,13 +16,17 @@ loader.lazyRequireGetter( const Types = require("devtools/client/inspector/flexbox/types"); +const { + highlightNode, + unhighlightNode, +} = require("devtools/client/inspector/boxmodel/actions/box-model-highlighter"); + class FlexItem extends PureComponent { static get propTypes() { return { + dispatch: PropTypes.func.isRequired, flexItem: PropTypes.shape(Types.flexItem).isRequired, index: PropTypes.number.isRequired, - onHideBoxModelHighlighter: PropTypes.func.isRequired, - onShowBoxModelHighlighterForNode: PropTypes.func.isRequired, scrollToTop: PropTypes.func.isRequired, setSelectedNode: PropTypes.func.isRequired, }; @@ -30,10 +34,9 @@ class FlexItem extends PureComponent { render() { const { + dispatch, flexItem, index, - onHideBoxModelHighlighter, - onShowBoxModelHighlighterForNode, scrollToTop, setSelectedNode, } = this.props; @@ -46,10 +49,10 @@ class FlexItem extends PureComponent { e.stopPropagation(); scrollToTop(); setSelectedNode(nodeFront); - onHideBoxModelHighlighter(); + dispatch(unhighlightNode()); }, - onMouseOut: () => onHideBoxModelHighlighter(), - onMouseOver: () => onShowBoxModelHighlighterForNode(nodeFront), + onMouseOut: () => dispatch(unhighlightNode()), + onMouseOver: () => dispatch(highlightNode(nodeFront)), }, dom.span({ className: "flex-item-index" }, index), getNodeRep(nodeFront) diff --git a/devtools/client/inspector/flexbox/components/FlexItemList.js b/devtools/client/inspector/flexbox/components/FlexItemList.js index fb57e79eb0db..6781f6c77eb9 100644 --- a/devtools/client/inspector/flexbox/components/FlexItemList.js +++ b/devtools/client/inspector/flexbox/components/FlexItemList.js @@ -21,22 +21,15 @@ const Types = require("devtools/client/inspector/flexbox/types"); class FlexItemList extends PureComponent { static get propTypes() { return { + dispatch: PropTypes.func.isRequired, flexItems: PropTypes.arrayOf(PropTypes.shape(Types.flexItem)).isRequired, - onHideBoxModelHighlighter: PropTypes.func.isRequired, - onShowBoxModelHighlighterForNode: PropTypes.func.isRequired, scrollToTop: PropTypes.func.isRequired, setSelectedNode: PropTypes.func.isRequired, }; } render() { - const { - flexItems, - onHideBoxModelHighlighter, - onShowBoxModelHighlighterForNode, - scrollToTop, - setSelectedNode, - } = this.props; + const { dispatch, flexItems, scrollToTop, setSelectedNode } = this.props; return dom.div( { className: "flex-item-list" }, @@ -53,10 +46,9 @@ class FlexItemList extends PureComponent { flexItems.map((flexItem, index) => FlexItem({ key: flexItem.actorID, + dispatch, flexItem, index: index + 1, - onHideBoxModelHighlighter, - onShowBoxModelHighlighterForNode, scrollToTop, setSelectedNode, }) diff --git a/devtools/client/inspector/flexbox/components/Flexbox.js b/devtools/client/inspector/flexbox/components/Flexbox.js index ef53320f65ce..ae1fb6a855c4 100644 --- a/devtools/client/inspector/flexbox/components/Flexbox.js +++ b/devtools/client/inspector/flexbox/components/Flexbox.js @@ -40,12 +40,11 @@ const Types = require("devtools/client/inspector/flexbox/types"); class Flexbox extends PureComponent { static get propTypes() { return { + dispatch: PropTypes.func.isRequired, flexbox: PropTypes.shape(Types.flexbox).isRequired, flexContainer: PropTypes.shape(Types.flexContainer).isRequired, getSwatchColorPickerTooltip: PropTypes.func.isRequired, - onHideBoxModelHighlighter: PropTypes.func.isRequired, onSetFlexboxOverlayColor: PropTypes.func.isRequired, - onShowBoxModelHighlighterForNode: PropTypes.func.isRequired, onToggleFlexboxHighlighter: PropTypes.func.isRequired, scrollToTop: PropTypes.func.isRequired, setSelectedNode: PropTypes.func.isRequired, @@ -53,18 +52,12 @@ class Flexbox extends PureComponent { } renderFlexItemList() { - const { - onHideBoxModelHighlighter, - onShowBoxModelHighlighterForNode, - scrollToTop, - setSelectedNode, - } = this.props; + const { dispatch, scrollToTop, setSelectedNode } = this.props; const { flexItems } = this.props.flexContainer; return FlexItemList({ + dispatch, flexItems, - onHideBoxModelHighlighter, - onShowBoxModelHighlighterForNode, scrollToTop, setSelectedNode, }); @@ -96,11 +89,11 @@ class Flexbox extends PureComponent { render() { const { + dispatch, + flexbox, flexContainer, getSwatchColorPickerTooltip, - onHideBoxModelHighlighter, onSetFlexboxOverlayColor, - onShowBoxModelHighlighterForNode, onToggleFlexboxHighlighter, setSelectedNode, } = this.props; @@ -113,15 +106,17 @@ class Flexbox extends PureComponent { } const { flexItemShown } = flexContainer; + const { color, highlighted } = flexbox; return dom.div( { className: "layout-flexbox-wrapper" }, Header({ + color, + dispatch, flexContainer, getSwatchColorPickerTooltip, - onHideBoxModelHighlighter, + highlighted, onSetFlexboxOverlayColor, - onShowBoxModelHighlighterForNode, onToggleFlexboxHighlighter, setSelectedNode, }), diff --git a/devtools/client/inspector/flexbox/components/Header.js b/devtools/client/inspector/flexbox/components/Header.js index 773b5a619b63..262ebd993d0c 100644 --- a/devtools/client/inspector/flexbox/components/Header.js +++ b/devtools/client/inspector/flexbox/components/Header.js @@ -12,7 +12,6 @@ const { } = require("devtools/client/shared/vendor/react"); const dom = require("devtools/client/shared/vendor/react-dom-factories"); const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); -const { connect } = require("devtools/client/shared/vendor/react-redux"); const { getStr } = require("devtools/client/inspector/layout/utils/l10n"); const FlexContainer = createFactory( @@ -27,12 +26,12 @@ const Types = require("devtools/client/inspector/flexbox/types"); class Header extends PureComponent { static get propTypes() { return { + color: PropTypes.string.isRequired, + dispatch: PropTypes.func.isRequired, flexContainer: PropTypes.shape(Types.flexContainer).isRequired, getSwatchColorPickerTooltip: PropTypes.func.isRequired, highlighted: PropTypes.bool.isRequired, - onHideBoxModelHighlighter: PropTypes.func.isRequired, onSetFlexboxOverlayColor: PropTypes.func.isRequired, - onShowBoxModelHighlighterForNode: PropTypes.func.isRequired, onToggleFlexboxHighlighter: PropTypes.func.isRequired, setSelectedNode: PropTypes.func.isRequired, }; @@ -75,19 +74,19 @@ class Header extends PureComponent { } const { + color, + dispatch, flexContainer, getSwatchColorPickerTooltip, - onHideBoxModelHighlighter, onSetFlexboxOverlayColor, - onShowBoxModelHighlighterForNode, } = this.props; return FlexContainer({ + color, + dispatch, flexContainer, getSwatchColorPickerTooltip, - onHideBoxModelHighlighter, onSetFlexboxOverlayColor, - onShowBoxModelHighlighterForNode, }); } @@ -142,10 +141,4 @@ class Header extends PureComponent { } } -const mapStateToProps = state => { - return { - highlighted: state.flexbox.highlighted, - }; -}; - -module.exports = connect(mapStateToProps)(Header); +module.exports = Header; diff --git a/devtools/client/inspector/inspector.js b/devtools/client/inspector/inspector.js index 748a57e07386..e74441e24010 100644 --- a/devtools/client/inspector/inspector.js +++ b/devtools/client/inspector/inspector.js @@ -149,7 +149,7 @@ function Inspector(toolbox) { this.panelWin = window; this.panelWin.inspector = this; this.telemetry = toolbox.telemetry; - this.store = createStore(); + this.store = createStore(this); this.isReady = false; // Map [panel id => panel instance] diff --git a/devtools/client/inspector/layout/components/LayoutApp.js b/devtools/client/inspector/layout/components/LayoutApp.js index 2e0a33ef1299..965f99f0d416 100644 --- a/devtools/client/inspector/layout/components/LayoutApp.js +++ b/devtools/client/inspector/layout/components/LayoutApp.js @@ -52,6 +52,7 @@ class LayoutApp extends PureComponent { static get propTypes() { return { boxModel: PropTypes.shape(BoxModelTypes.boxModel).isRequired, + dispatch: PropTypes.func.isRequired, flexbox: PropTypes.shape(FlexboxTypes.flexbox).isRequired, getSwatchColorPickerTooltip: PropTypes.func.isRequired, grids: PropTypes.arrayOf(PropTypes.shape(GridTypes.grid)).isRequired, diff --git a/devtools/client/inspector/store.js b/devtools/client/inspector/store.js index 79340883f07f..ad2cbcdac78e 100644 --- a/devtools/client/inspector/store.js +++ b/devtools/client/inspector/store.js @@ -20,11 +20,13 @@ function createReducer(laterReducers = {}) { }); } -module.exports = () => { +module.exports = inspector => { const store = createStore(createReducer(), { // Enable log middleware in tests shouldLog: true, - thunkOptions: {}, + // Pass the client inspector instance so thunks (dispatched functions) + // can access it from their arguments + thunkOptions: { inspector }, }); // Map of registered reducers loaded on-demand.