Profiler: Support "reload and start profiling" option

[bvaughn]

Either resume recording if the DevTools (or a web page) are re-loaded while recording is active, or add a restart-and-profile button like Chrome "Performance" tab has. (This may not be possible due to initialization/timing issues.)

See this comment for some work in progress...

[sag1v]

Is there a way (maybe via the backend's mount) to pass props to the root?
same as you can call multiple times to ReactDOM.render(...). then if we pass a key, it will force it to remount.
just a thought.

[bvaughn]

Forcing a remount seems sketchy. It could cause side effects to be applied twice which could e.g. break a page, double-log data somewhere remotely– all sorts of things. It could also be pretty slow.

[sag1v]

I meant attach it to a button that will explicit fire a re-mount and record. But if you think this is not the way to go then fair enough

[bvaughn]

Gotcha. Still seems questionable to me. e.g. What if there are multiple React roots on the page? Or multiple versions of React?

[sag1v]

Well how does the profiler handle multiple roots or versions now?
I think its per instance id, maybe display a dropdown to choose the desired root?

[mehiel]

@bvaughn would an approach like that work?
mehiel@00808d9

Yes we need to take care of several configurations, multiple roots etc. but I think there are solutions to these problems. The main thing on that approach would be to discuss if the handshake between the render method and the profiler is something that would be of interest.

[bvaughn]

I'm going to paste what I said on Twitter for anyone who sees this thread:

At a glance– I'm not sure how much value this adds over the Codepen I shared the other day. React can't always wait for profiler, because DevTools might not even be installed– so you'd have to modify your app code to use this new wait-for-render behavior only when you know you're going to be using the Profiler. At this point, why not just use an approach like the one I shared yesterday?

You responded:

react could always have the same logic (if devtools are not there it renders immediately)

Are we guaranteed that DevTools will always load/initialize before JS on the page?

I think so based on my own testing, and interpretation of the docs, but it would be nice to definitely know. Otherwise React rendering immediately if the DevTools weren't present would defeat the purpose of waiting.

The Mozilla docs say this about the timing of the content scripts run_at setting that we use:

"document_start": corresponds to loading. The DOM is still loading.

I think this confirms my assumption, but that being said, it looks like the DevTools extension was written under the assumption that React might load onto the page before the extension does. Maybe this used to be the case? Or maybe it was a misunderstanding (seems less likely)?

The main thing on that approach would be to discuss if the handshake between the render method and the profiler is something that would be of interest.

To be honest– this still doesn't feel like a good idea.

If the React DevTools is guaranteed to run before React runs on the page, I'd rather invest energy into getting rid of the plugins initialization delay (or finding some other way) so that the Profiler can just start profiling immediately, before the first commit. I think there's an async delay with the bridge traffic currently that we'd have to work around, but maybe it would be possible to do this somehow? I haven't thought that much about it.

[mehiel]

Sure if sync preparation of the profiler can be assured it should be done that way, there is no reason to handshake and make it more complex than it should be.

But if not I can't see any other way to support it.

Are we guaranteed that DevTools will always load/initialize before JS on the page?

No and it doesn't matter. The flow of ReactDOM.render could go like:

1. Are DevTools available (by checking the existence of the hook)?
   if NO: render immediately (as is in the current version of ReactDOM)

2. Is the option hook.profileOnReload === true?
   if NO: render immediately

3. Render a fallback element and call hook.waitForProfiler() to get the promise that is resolved when profiler is ready. So either the profiler is initialized before the page and the promise is already resolved or it waits till the profiler gets ready.

4. When the profiler is ready the promise is resolved and render continues to mount the actual component.

5. The profiler is already recording the snapshots on commit and then you can stop on demand.

For a better UX if profileOnReload === true we also preselect the Profler tab. Now especially in environments like CRA, I only have to edit my code, page reloads on change and I just press stop to see what happened. Not happy? Repeat. No extra clicks on devtools besides stopping the profiler.

[mehiel]

When it comes to the hook I think yes, the hook will be available before the page. AFAICT that's the point of:
https://github.com/facebook/react-devtools/blob/master/shells/webextension/src/GlobalHook.js#L73

Is this correct?

And maybe that's a way to delay the rendering of the rest of the document as well to achieve the behavior you described above? Not sure but seems a bit brutal.

Also as pointed in the Readme file, there are other use cases that devtools require the hook to be loaded before the page. For example in order to read the original implementations of Set and Map to avoid any monkeypatching made by the user.
https://github.com/facebook/react-devtools/blob/master/shells/webextension/Readme.md#L11

[mehiel]

Are we guaranteed that DevTools will always load/initialize before JS on the page?

I think that DevTools are not "atomic" when it comes to initialization. The backend and the hook injection seem that are guaranteed to execute before any other script in the page. But the content scripts may be not.

[bvaughn]

The hook is the part that would need to be guaranteed to run before React does for an approach like the one you proposed to be potentially viable. Assuming Jared was right when he wrote those lines in 41f5760, then that would be the case.

I'm still not a fan of increasing the surface area– (we'd need to add two new methods– one for render and one for createRoot)– and complexity of the React DOM renderer as a solution to this. I still think this CodeSandbox is a preferable approach.

[mehiel]

That's on you and it'd be a totally respectable decision to not go that way.

All I'm saying is that it's something technically feasible without much new code but unfortunately by somehow coupling the render method with the devtools and the profiler.

[bvaughn]

Fair enough. I believe you're right that it's technically feasible 😄 Thanks for the discussion!