1409064 - Network Monitor needs to use ES6 classes so that we can switch it over to PureComponent

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

jlast pointed me at https://github.com/reactjs/react-codemod that can help automate these changes.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The codemod jlast pointed me at uses ES6 class arrow functions, static properties and a bunch of other features that are currently only supported by babel.

I have made significant changes to the AST to get it working with our code base but I think it is fine... at least, after converting the Network Monitor it still seems to work fine.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ae8a43b7403a523d7e004ca6fe8a9735e6174cc

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1409064 - Network Monitor swith to ES6 classes

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

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

try is green so asked Honza for review.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Mike: The patch needs to be rebased.

unable to find 'devtools/client/netmonitor/src/components/headers-panel.js' for patching
4 out of 4 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/headers-panel.js.rej
unable to find 'devtools/client/netmonitor/src/components/monitor-panel.js' for patching
4 out of 4 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/monitor-panel.js.rej
unable to find 'devtools/client/netmonitor/src/components/properties-view.js' for patching
5 out of 5 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/properties-view.js.rej
unable to find 'devtools/client/netmonitor/src/components/request-list-column-cause.js' for patching
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/request-list-column-cause.js.rej
unable to find 'devtools/client/netmonitor/src/components/request-list-column-content-size.js' for patching
etc..

The file-names changed. E.g. headers-panel.js => HeadersPanel.js
(to follow CamelCase standard for React component files)


Honza

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

No problem... the network panel was converted in less than 10 seconds so I can recreate the patch any time.

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8920156 [details]
Bug 1409064 - Network Monitor swith to ES6 classes

https://reviewboard.mozilla.org/r/191124/#review197108

::: devtools/client/netmonitor/src/components/headers-panel.js:276
(Diff revision 1)
>        )
>      );
>    }
> -});
> +}
> +
> +HeadersPanel.displayName = "HeadersPanel";

displayName is useless after converting to ES6 Classes. Please remove it.

https://reactjs.org/docs/react-component.html#displayname

::: devtools/client/netmonitor/src/components/headers-panel.js:278
(Diff revision 1)
>    }
> -});
> +}
> +
> +HeadersPanel.displayName = "HeadersPanel";
> +
> +HeadersPanel.propTypes = {

Can we move propType into Component and use static prop instead?


class HeadersPanel extends Component {
  static defaultProp = {
    ...
  };

  static propTypes = {
    ...
  };
}

Plus, we can also define `state` out of constructor like this:

class HeadersPanel extends Component {
  state = {
    ...
  };
}

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8920156 [details]
Bug 1409064 - Network Monitor swith to ES6 classes

https://reviewboard.mozilla.org/r/191124/#review197108

> displayName is useless after converting to ES6 Classes. Please remove it.
> 
> https://reactjs.org/docs/react-component.html#displayname

It isn't needed when using createClass() either.

We can't remove it without the team agreeing that we will no longer use it so I have created an RFC:
https://github.com/devtools-html/rfcs/issues/25

> Can we move propType into Component and use static prop instead?
> 
> 
> class HeadersPanel extends Component {
>   static defaultProp = {
>     ...
>   };
> 
>   static propTypes = {
>     ...
>   };
> }
> 
> Plus, we can also define `state` out of constructor like this:
> 
> class HeadersPanel extends Component {
>   state = {
>     ...
>   };
> }

> Can we move propType into Component and use static prop instead?
> 
> class HeadersPanel extends Component {
>   static defaultProp = {
>     ...
>   };
> 
> static propTypes = {
>     ...
>   };
> }

No, the majority of our code is not transpiled and class properties are not part of ES7. From what I understand, this is because static getters are supported.

I have changed the AST transformer to use static getters instead.
> Plus, we can also define state out of constructor like this:
> 
> class HeadersPanel extends Component {
>   state = {
>     ...
>   };
> }

Again, class properties are not part of ES7 so we can't do that.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1409064 - Network Monitor to ES6 classes

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

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8921435 [details]
Bug 1409064 - Network Monitor to ES6 classes

https://reviewboard.mozilla.org/r/192464/#review198082

I like the patch, thanks Mike!

R+ assuming try is green.

Honza

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a6a0703724e
Network Monitor to ES6 classes r=Honza

Comment 12

[Attila Craciun [:arny]]

https://hg.mozilla.org/mozilla-central/rev/5a6a0703724e