1207997 - Consider wrapping about:debugging UI components with `createFactory`.

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Jan Keromnes [:janx]]

(James Long (:jlongster) in bug 1196785 comment 25)
> Oh I see, you literally call `React.createElement`. It works for now, but it
> is quite verbose and I would recommend separating the ui components into
> different files in a `components` folder and wrapping them with
> `createFactory` when importing them. I don't know if we should block this on
> that or not, at least file a follow-up bug I guess.

This is the suggested follow-up bug.

Comment 1

[Jan Keromnes [:janx]]

James, about:debugging landed, and calls `React.createElement` for all the required components.

What would be the benefit of changing this to `createFactory`? (I've never done this before, and don't have much experience with React apart from the about:debugging components.)

Comment 2

[James Long (:jlongster)]

Hey Jan, sorry for not replying earlier.

The benefit is just less verbosity. Going from:

React.createElement(
  Toolbar,
  { links: ['one', 'two', 'three'] },
  React.createElement(Button, { label: 'one' }),
  React.createElement(Button, { label: 'two' }),
  React.createElement(Button, { label: 'three' })
);

To:

Toolbar({ links: ['one', 'two', 'three'] },
        Button({ label: 'one' }),
        Button({ label: 'two' }),
        Button({ label: 'three' }))

Is a lot cleaner, and the structure is more apparent. We're trying to decide if we should use JSX, because even the latter isn't as clean as JSX. But that's a ways off if we ever do it because we'll need a build step.

I hope I'm understanding your question correctly.

Using `createFactory` turns a component class into a callable function. You should do this whenever you import a component:

const Toolbar = React.createFactory(require('toolbar'));

And then you can call it like `Toolbar()` now. It makes importing a little verbose, but it's not too bad.

Comment 3

[James Long (:jlongster)]

Note that I believe several places where we are writing React (Jordan's memory tool, and I think Honza's work) already does this so it's already pretty standard (folks contributing React will expect this too).

Comment 4

[Jan Keromnes [:janx]]

Alex, since you're rewriting some of about:debugging's React code, please consider James' suggestion for any new code you're adding.

Comment 5

[Jan Keromnes [:janx]]

I have some refactoring in progress, so I'll try to sneak this in.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1207997 - about:debugging use factories/dom helpers instead of createElement;r=janx

Review commit: https://reviewboard.mozilla.org/r/36635/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36635/

Comment 7

[Jan Keromnes [:janx]]

Comment on attachment 8723584 [details]
MozReview Request: Bug 1207997 - about:debugging use factories/dom helpers instead of createElement;r=janx

https://reviewboard.mozilla.org/r/36635/#review33369

Using `createFactory` does make the code a lot more readable, thanks a lot for the patch Julian!

Mostly silly nits, but I think you forgot to change the references to `React` in `initializer.js`.

::: devtools/client/aboutdebugging/components/addons-controls.js:15
(Diff revision 1)
> -const React = require("devtools/client/shared/vendor/react");
> +const { createClass, DOM: dom} = require("devtools/client/shared/vendor/react");

Nit: Please add a space between `dom` and `}`.

::: devtools/client/aboutdebugging/components/addons-controls.js:29
(Diff revision 1)
> -    return React.createElement(
> -      "div", { className: "addons-controls" }, React.createElement(
> +    return dom.div(
> +        { className: "addons-controls" },

Nit: When there is space, please place the `props` object on the same line as the created element.

::: devtools/client/aboutdebugging/components/addons-tab.js:12
(Diff revision 1)
> -const { TabHeader } = require("./tab-header");
> -const { TargetList } = require("./target-list");
> +const TargetList = createFactory(require("./target-list"));
> +const TabHeader = createFactory(require("./tab-header"));
> +const AddonsControls = createFactory(require("./addons-controls"));

Nit: Please sort the `createFactory` lines alphabetically (`AddonsControls` comes first).

::: devtools/client/aboutdebugging/components/addons-tab.js:58
(Diff revision 1)
> -    return React.createElement(
> +    return dom.div({ id: "tab-addons", className: "tab", role: "tabpanel",
> -      "div", { id: "tab-addons", className: "tab", role: "tabpanel",
>          "aria-labelledby": "tab-addons-header-name" },

Nit: Our styleguide for `props` arguments that don't fit in a single line is inconsistent.

What about we always split them one prop per line if everything doesn't fit in one line? E.g.

dom.div({
  id: "tab-addons",
  className: "tab",
  role: "tabpanel",
  /* ... */
})

::: devtools/client/aboutdebugging/components/addons-tab.js:60
(Diff revision 1)
> -        React.createElement(TabHeader, {
> -          id: "tab-addons-header-name",
> +        TabHeader({ id: "tab-addons-header-name",
> +          name: Strings.GetStringFromName("addons") }),

Nit: Please split the `props` argument one prop per line.

::: devtools/client/aboutdebugging/components/workers-tab.js:13
(Diff revision 1)
> -const { TabHeader } = require("./tab-header");
> +const TargetList = createFactory(require("./target-list"));
> +const TabHeader = createFactory(require("./tab-header"));

Nit: Please sort the `createFactory` lines alphabetically.

::: devtools/client/aboutdebugging/components/workers-tab.js:52
(Diff revision 1)
> -    return React.createElement(
> +    return dom.div({ id: "tab-workers", className: "tab", role: "tabpanel",
> -      "div", { id: "tab-workers", className: "tab", role: "tabpanel",
>          "aria-labelledby": "tab-workers-header-name" },

Nit: Please split the `props` argument one prop per line.

::: devtools/client/aboutdebugging/initializer.js:40
(Diff revision 1)
>        let app = React.createElement(AboutDebuggingApp, { client, telemetry });
>        React.render(app, document.querySelector("#body"));

Nit: Oops, you forgot a call to `React.createElement` here.

Nit: Also, `React` is no longer imported in this file (even though it seems to exist in the global scope), so please either add it to ESLint's `/* global */` list or remove its references from this file.

Comment 8

[Jan Keromnes [:janx]]

The code definitely looks much cleaner now, thanks for the patch Julian!

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8723584 [details]
MozReview Request: Bug 1207997 - about:debugging use factories/dom helpers instead of createElement;r=janx

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36635/diff/1-2/

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1e55f160b4ae

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e55f160b4ae