firefox-devtools / rfcs

RFCs for changes to DevTools
15 stars 17 forks source link

HOCs proposal for performance analysis #34

Closed MikeRatcliffe closed 6 years ago

MikeRatcliffe commented 6 years ago

In order to better be able to optimize our React Components I would like to propose adding some Higher Order Components (HOCs).

I have just had a long conversation on #reactjs and it seems that the only way to automatically add a chain of HOCs is via monkeypatching... I wanted to avoid that but at least I know how to proceed. This will allow us to use whydidyouupdate, objectdiff etc.

Proposed HOCs

whydidyouupdate Bug 1390093

This mixin notifies you in the console when potentially unnecessary re-renders occur:

whydidyouupdate1 whydidyouupdate2

objectdiff Bug 1393515

This mixin shows exactly which properties have changed in a component on each render. As an example see the image below. From the image you can see that in the ToolboxToolbar component we are inserting button ids one by one. It would obviously be more performant to gather all of these changes together and add them all at once:

objectdiff

Issues

Questions

  1. Does anybody have a way they would prefer me to do this? After long conversations on #reactjs the only way to apply a chain of HOCs to all components is to monkeypatch React itself.
  2. Are there any other mixins / HOCs that you would like me to add to this toolset?
  3. Is anybody vehemently opposed to me doing this?
  4. Does anybody not see the point of doing this? The last thing I want to do is take weeks implementing something that nobody wants.
sole commented 6 years ago

Hello! Thanks for filing the RFC!

I have no firm opinion on what to do, but a few questions :-)

1) Would this be enabled in debug builds, or behind a flag, or...? 2) When you say monkey patch, do you mean you're going to edit the source code in place (not very maintainable on the long term) or you're going to dynamically inject stuff into the React instance when initialising it? 3) Also why do you need to do it "everywhere" React is used? Which ones are these instances? Is it debugger-html and then devtools in m-c? I can't imagine having to use this on the launchpad, for example. 4) Is there any way to do this 'middleware' style like in Redux? So you don't "monkey patch" but rather use an 'official' way to alter React's behaviour.

Thanks!

MikeRatcliffe commented 6 years ago
digitarald commented 6 years ago

Is there any way to do this 'middleware' style like in Redux? So you don't "monkey patch" but rather use an 'official' way to alter React's behaviour.

Redux, react-router and others use React's context: https://reactjs.org/docs/context.html ; which would allow handing down data without monkeypatching. I have not used them extensively, but @bgrins and I did some exercises with them during react training.

MikeRatcliffe commented 6 years ago

Redux, react-router and others use React's context: https://reactjs.org/docs/context.html ; which would allow handing down data without monkeypatching. I have not used them extensively, but @bgrins and I did some exercises with them during react training.

Sadly, HOCs need to wrap Component or PureComponent just before a component is returned from a require file and can be chained by wrapping the result e.g. return hoc1(hoc2(hoc3(Component))).

This can't be done from the context property. It looks like we will need to patch react.js but the code would be very simple, something like e.g.

if (hoc1Pref) {
  Component  = hoc1(Component)
}
if (hoc2Pref) {
  Component  = hoc2(Component)
}
if (hoc3Pref) {
  Component  = hoc3(Component)
}
Return Component; // Same for PureComponent
ochameau commented 6 years ago

Does anybody not see the point of doing this? The last thing I want to do is take weeks implementing something that nobody wants.

I would phrase that differently. Is there an audience? Did your current experiments helped you or someone else to write a patch to improve our component implementations? If yes, did that actually improved the performance? Like on DAMP/profiler/local timings?

Also, I'm not a React expert, but it looks like such tool would become useless if we move to PureComponent, right? To me, this tooling is made to optimize shouldComponentUpdate methods which are no longer relevant with PureComponents. We are already using them in a bunch of component on m-c, are we planning to convert all the others?

Finaly, isn't there a way you could contribute this feature somehow to react devtools instead of making a tool builtin and very specific to our codebase? Is react devtools able to implement all of its features with public react API, or are they hacking into it?

MikeRatcliffe commented 6 years ago

I had a bunch of changes that produced greater local timings in Q3 but because the codebase was changing so rapidly, especially the Web Console, I just never had chance to land them. Of course, the higher up the component tree the changes the better the result.

Also, I'm not a React expert, but it looks like such tool would become useless if we move to PureComponent, right?

That is mostly true but PureComponent only works whenever the rendering of your component depends only on props and state. This should always be the case in react but there are some examples where you need to re-render using lifecycle methods in which case we can't use PureComponent.

Finaly, isn't there a way you could contribute this feature somehow to react devtools instead of making a tool builtin and very specific to our codebase?

No, they are very specific about what they want inside React DevTools and they don't want to add this kind of thing.

Is react devtools able to implement all of its features with public react API, or are they hacking into it?

There is a hook that makes it possible to connect up but it is really designed only for React DevTools and has a very limited API.

MikeRatcliffe commented 6 years ago

I should point out that when we upgrade to React 16 the information inside perf.html will be much easier to read. The React Team say this better use of the User Timing API is a decent replacement for React.Perf.

react 16 in perf html

juliandescottes commented 6 years ago

Quickly discussed this with @MikeRatcliffe .

React 16 will come with new markers and perf information that could make this work redundant (see comment above). Knowing that, and knowing that working on such tooling would take a lot of time, it is probably better to wait until DevTools are using React 16 and then decide if we need additional tooling or not.

DevTools migration to React 16 is tracked in https://bugzilla.mozilla.org/show_bug.cgi?id=1416824

julienw commented 6 years ago

Just a quick remark: even with PureComponent, we can have spurious updates (when a prop changed but we hadn't expected it), so a tool that tells why can be useful. That said not all updates are equals and the time to do a specific update is also useful. What can help track perf issues is the alliance of both.

But I agree that while the timings come for free, the HOC devised here is a lot of work, so I think like @juliandescottes above.

juliandescottes commented 6 years ago

Closing the RFC for now, will be reassessed after Migration to React 16 is completed.