Closed Bug 1412311 Opened 7 years ago Closed 7 years ago

DevTools Shared Components needs to use ES6 classes so that we can switch it over to PureComponent

Categories

(DevTools :: Shared Components, enhancement, P2)

enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: miker, Assigned: miker)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
devtools/client/shared/components/Tree.js is the difficult one here because it has a whole host of issues preventing it from being converted.
devtools/client/shared/components/tree/TreeView.js has a special case... After a weekend of debugging I have discovered that this pattern does not work: ``` class TreeView extends Component { static get defaultProps() { return { object: null, renderRow: null, provider: ObjectProvider, expandedNodes: new Set(), expandableStrings: true, columns: [] }; } ... } ``` It turns out that this is because a static getter returns a new object every time whereas a static property will return the same property every time. I have worked around this by following this pattern so that we can return the same object every time: ``` const defaultProps = { object: null, renderRow: null, provider: ObjectProvider, expandedNodes: new Set(), expandableStrings: true, columns: [] }; class TreeView extends Component { static get defaultProps() { return defaultProps; } ... } ```
Actually, this shows that we are mutating props somewhere and that is bad practice... investigating.
I have logged bug 1413167 for devtools/client/shared/components/tree/TreeView.js, which is somehow changing it's defaultProps.
Comment on attachment 8923793 [details] Bug 1412311 - DevTools Shared Components to ES6 classes https://reviewboard.mozilla.org/r/194926/#review199992 Thanks for taking care of that Mike. The codemod seems to have deleted more than it should, can this be fixed please ? ::: devtools/client/shared/components/Tree.js:13 (Diff revision 1) > -/** > - * A fast, generic, expandable and collapsible tree component. > - * > +class Tree extends Component { > + static get propTypes() { > + return { > - * This tree component is fast: it can handle trees with *many* items. It only > - * renders the subset of those items which are visible in the viewport. It's > - * been battle tested on huge trees in the memory panel. We've optimized tree > - * traversal and rendering, even in the presence of cross-compartment wrappers. > - * > - * This tree component doesn't make any assumptions about the structure of your > - * tree data. Whether children are computed on demand, or stored in an array in > - * the parent's `_children` property, it doesn't matter. We only require the > - * implementation of `getChildren`, `getRoots`, `getParent`, and `isExpanded` > - * functions. > - * > - * This tree component is well tested and reliable. See > - * devtools/client/shared/components/test/mochitest/test_tree_* and its usage in > - * the performance and memory panels. > - * > - * This tree component doesn't make any assumptions about how to render items in > - * the tree. You provide a `renderItem` function, and this component will ensure > - * that only those items whose parents are expanded and which are visible in the > - * viewport are rendered. The `renderItem` function could render the items as a > - * "traditional" tree or as rows in a table or anything else. It doesn't > - * restrict you to only one certain kind of tree. > - * > - * The only requirement is that every item in the tree render as the same > - * height. This is required in order to compute which items are visible in the > - * viewport in constant time. > - * > - * ### Example Usage > - * > - * Suppose we have some tree data where each item has this form: > - * > - * { > - * id: Number, > - * label: String, > - * parent: Item or null, > - * children: Array of child items, > - * expanded: bool, > - * } > - * > - * Here is how we could render that data with this component: > - * > - * const MyTree = createClass({ > - * displayName: "MyTree", > - * > - * propTypes: { > - * // The root item of the tree, with the form described above. > - * root: PropTypes.object.isRequired > - * }, > - * > - * render() { > - * return Tree({ > - * itemHeight: 20, // px > - * > - * getRoots: () => [this.props.root], > - * > - * getParent: item => item.parent, > - * getChildren: item => item.children, > - * getKey: item => item.id, > - * isExpanded: item => item.expanded, > - * > - * renderItem: (item, depth, isFocused, arrow, isExpanded) => { > - * let className = "my-tree-item"; > - * if (isFocused) { > - * className += " focused"; > - * } > - * return dom.div( > - * { > - * className, > - * // Apply 10px nesting per expansion depth. > - * style: { marginLeft: depth * 10 + "px" } > - * }, > - * // Here is the expando arrow so users can toggle expansion and > - * // collapse state. > - * arrow, > - * // And here is the label for this item. > - * dom.span({ className: "my-tree-item-label" }, item.label) > - * ); > - * }, > - * > - * onExpand: item => dispatchExpandActionToRedux(item), > - * onCollapse: item => dispatchCollapseActionToRedux(item), > - * }); > - * } > - * }); > - */ This shouldn't be removed ::: devtools/client/shared/components/Tree.js:152 (Diff revision 1) > + this._onExpand = oncePerAnimationFrame(this._onExpand); > + this._onCollapse = oncePerAnimationFrame(this._onCollapse); > + this._onScroll = oncePerAnimationFrame(this._onScroll); > + this._focusPrevNode = oncePerAnimationFrame(this._focusPrevNode); > + this._focusNextNode = oncePerAnimationFrame(this._focusNextNode); > + this._focusParentNode = oncePerAnimationFrame(this._focusParentNode); maybe we can bind them right away (`this._onExpand = oncePerAnimationFrame(this._onExpand).bind(this);`) ? ::: devtools/client/shared/components/Tree.js:598 (Diff revision 1) > nodes > ); > } > -}); > +} > + > +module.exports = Tree; could this be moved at the end of the file ? It's kind of the pattern we follow if I'm right ::: devtools/client/shared/components/splitter/SplitBox.js:54 (Diff revision 1) > - /** > - * The state stores the current orientation (vertical or horizontal) > - * and the current size (width/height). All these values can change > - * during the component's life time. > - */ > + constructor(props) { > + super(props); > + > + this.state = { > + vert: props.vert, keep the comment ::: devtools/client/shared/components/tree/TreeCell.js (Diff revision 1) > // Make this available to both AMD and CJS environments > define(function (require, exports, module) { > - const React = require("devtools/client/shared/vendor/react"); > - > + const { Component, DOM: dom, PropTypes } = > + require("devtools/client/shared/vendor/react"); > - // Shortcuts > - const { input, span, td } = React.DOM; can we keep this line ? (so we don't change the "content" of the file ) const { input, span, td } = dom; ::: devtools/client/shared/components/tree/TreeHeader.js (Diff revision 1) > define(function (require, exports, module) { > - // ReactJS > - const React = require("devtools/client/shared/vendor/react"); > + const { Component, DOM: dom, PropTypes } = > + require("devtools/client/shared/vendor/react"); > - > - // Shortcuts > - const { thead, tr, td, div } = React.DOM; Same here, can we keep the shortcuts ::: devtools/client/shared/components/tree/TreeRow.js (Diff revision 1) > > // Scroll > const { scrollIntoViewIfNeeded } = require("devtools/client/shared/scroll"); > > - // Shortcuts > - const { tr } = React.DOM; ditto
Attachment #8923793 - Flags: review?(nchevobbe) → review-
Comment on attachment 8923793 [details] Bug 1412311 - DevTools Shared Components to ES6 classes https://reviewboard.mozilla.org/r/194926/#review199992 RE: The shortcuts e.g. const { tr } = React.DOM; I was trying to make life easier on myself for when I remove all of the warnings but keeping them works just fine.
Comment on attachment 8923793 [details] Bug 1412311 - DevTools Shared Components to ES6 classes https://reviewboard.mozilla.org/r/194926/#review200864 This looks good, let's land it if TRY is green
Attachment #8923793 - Flags: review?(nchevobbe) → review+
Pushed by mratcliffe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3fa859a78370 DevTools Shared Components to ES6 classes r=nchevobbe
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: