1441218 - Restore original init code to font inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement)

Description

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

It is not a React component and React complained that we use this.store.dispatch() from the constructor.

Everything works fine as it is so we just need to remove the componentWillMount method.

Comment 1

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

Attachment: Bug 1441218 - Restore original init code to font inspector

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8954072 [details]
Bug 1441218 - Restore original init code to font inspector

https://reviewboard.mozilla.org/r/223222/#review229474

::: devtools/client/inspector/fonts/fonts.js
(Diff revision 1)
> -    this.store.dispatch(updatePreviewText(""));
> -    this.update(false, "");

shouldn't we move those into the init function, where they were located before ?

Comment 3

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

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)
> Comment on attachment 8954072 [details]
> Bug 1441218 - Remove componentWillMount() from the font inspector
> 
> https://reviewboard.mozilla.org/r/223222/#review229474
> 
> ::: devtools/client/inspector/fonts/fonts.js
> (Diff revision 1)
> > -    this.store.dispatch(updatePreviewText(""));
> > -    this.update(false, "");
> 
> shouldn't we move those into the init function, where they were located
> before ?

Absolutely not... I do we get React warnings saying you can't call them from the constructor.
And the Component works absolutely fine without those two lines.

I'll resubmit it for review.

Comment 4

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

Comment on attachment 8954072 [details]
Bug 1441218 - Restore original init code to font inspector

nchevobbe is nervous about removing those two lines of code in case there are edge cases that we have missed. I will investigate further before deciding what to do.

Comment 5

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

This is the original warning message:

`Warning: setState(...): Cannot update during an existing state transition (such as within `render` or another component's constructor). Render methods should be a pure function of props and state; constructor side-effects are an anti-pattern, but can be moved to `componentWillMount`.`

Comment 6

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

Comment on attachment 8954072 [details]
Bug 1441218 - Restore original init code to font inspector

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

Comment 7

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

Turns out that this is a redux issue that was fixed by the redux update ( bug 1441147 ).

This patch moves the original init code back into place as the reviewer requested.

Comment 8

[Patrick Brosset <:pbro>]

If you want to know for sure if removing/moving these 2 lines will cause a problem, I suggest asking Gabriel who wrote this part of the code.

Comment 9

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

(In reply to Patrick Brosset <:pbro> from comment #8)
> If you want to know for sure if removing/moving these 2 lines will cause a
> problem, I suggest asking Gabriel who wrote this part of the code.

There is no error with the lines in place because of the redux update so it is simply an issue of putting the lines back.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8954072 [details]
Bug 1441218 - Restore original init code to font inspector

https://reviewboard.mozilla.org/r/223222/#review229798

Thanks Mike

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c518398e5fd1
Remove componentWillMount() from the font inspector r=nchevobbe

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c518398e5fd1