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)
DevTools
Shared Components
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.
Assignee | ||
Comment 1•7 years ago
|
||
devtools/client/shared/components/Tree.js is the difficult one here because it has a whole host of issues preventing it from being converted.
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
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;
}
...
}
```
Assignee | ||
Comment 4•7 years ago
|
||
Actually, this shows that we are mutating props somewhere and that is bad practice... investigating.
Assignee | ||
Comment 5•7 years ago
|
||
I have logged bug 1413167 for devtools/client/shared/components/tree/TreeView.js, which is somehow changing it's defaultProps.
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
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+
Comment 11•7 years ago
|
||
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3fa859a78370
DevTools Shared Components to ES6 classes r=nchevobbe
![]() |
||
Comment 12•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•