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)

Product:
DevTools
Component:
Shared Components
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: miker, Assigned: miker)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1412311 - DevTools Shared Components to ES6 classes
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
      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
https://hg.mozilla.org/mozilla-central/rev/3fa859a78370
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
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

Creator:
Created:
Updated:
Size: