Closed Bug 1441218 Opened 4 years ago Closed 4 years ago

Restore original init code to font inspector

Categories

(DevTools :: Inspector, enhancement)

enhancement
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file)

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.
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
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 ?
Attachment #8954072 - Flags: review?(nchevobbe) → review-
(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.
Attachment #8954072 - Flags: review- → review?(nchevobbe)
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.
Attachment #8954072 - Flags: review?(nchevobbe)
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`.`
Summary: Remove componentWillMount() from the font inspector → Restore original init code to font inspector
Attachment #8954072 - Attachment description: Bug 1441218 - Remove componentWillMount() from the font inspector → Bug 1441218 - Restore original init code to font inspector
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.
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.
(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 on attachment 8954072 [details]
Bug 1441218 - Restore original init code to font inspector

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

Thanks Mike
Attachment #8954072 - Flags: review?(nchevobbe) → review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c518398e5fd1
Remove componentWillMount() from the font inspector r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/c518398e5fd1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.