Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix[react-devtools]: removed redundant startProfiling call #31131

Merged

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Oct 7, 2024

Stacked on #31118. See last commit.

We don't need to call startProfiling() here, because we delegate this to the Renderer itself:

// Automatically start profiling so that we don't miss timing info from initial "mount".
if (reloadAndProfileConfig.shouldReloadAndProfile) {
const shouldRecordChangeDescriptions =
reloadAndProfileConfig.recordChangeDescriptions;
startProfiling(shouldRecordChangeDescriptions);
}

Since this is de-facto the constructor of Renderer, this will be called earlier.

Validated via testing the reload-to-profile for Chrome browser extension.

Copy link

vercel bot commented Oct 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 7, 2024 8:25am

Copy link
Contributor

@EdmondChuiHW EdmondChuiHW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we observed unexpected/harmful behaviour from this call? I'm leaning towards "if it ain't broke don't fix" without the context

@hoxyq
Copy link
Contributor Author

hoxyq commented Oct 9, 2024

Have we observed unexpected/harmful behaviour from this call? I'm leaning towards "if it ain't broke don't fix" without the context

No, because we have a checks inside both startProfiling() and stopProfiling() that make them idempotent.

The main reason why I am removing it here, because we shouldn't be storing profiling settings in Agent, we can skip this step and just pass it from Hook -> Renderer. This is what is implemented in #31132.

@hoxyq hoxyq merged commit 4a86ec5 into facebook:main Oct 9, 2024
184 checks passed
@hoxyq hoxyq deleted the react-devtools/remove-redundant-profiling-call branch October 9, 2024 12:32
hoxyq added a commit that referenced this pull request Oct 9, 2024
Stacked on #31131. See last
commit.

This is a clean-up and a pre-requisite for next changes:
1. `ReloadAndProfileConfig` is now split into boolean value and settings
object. This is mainly because I will add one more setting soon, and
also because settings might be persisted for a longer time than the flag
which signals if the Backend was reloaded for profiling. Ideally, this
settings should probably be moved to the global Hook object, same as we
did for console patching.
2. Host is now responsible for reseting the cached values, Backend will
execute provided `onReloadAndProfileFlagsReset` callback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants